Skip to content

Reduce the size of the APIHooks, eliminating enum gap #9509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zwoop merged 1 commit into from
Mar 12, 2023
Merged

Reduce the size of the APIHooks, eliminating enum gap #9509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/ts/apidefs.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ typedef enum {

// Putting the SSL hooks in the same enum space
// So both sets of hooks can be set by the same Hook function
TS_SSL_FIRST_HOOK = 201,
TS_SSL_FIRST_HOOK,
Copy link
Contributor

@bneradt bneradt Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe my intentions were misplaced, but I intentionally added this gap here to allow room for new TS_HTTP_* values without breaking compatibility:
#8066

Note that any new TS_HTTP_* value requires a corresponding TS_EVENT_HTTP_* value at the same offset. But that's not the case for TS_SSL_* values. Therefore, any new TS_HTTP_* value added must be added before the TS_SSL_* values, and therefore, without the gap, must break compatibility because all subsequent TS_SSL values would be shifted. This was not understood when someone added TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK at the end of this enum after TS_SSL_*, which I'm sure was done to try to not break compatibility. I had to fix that in #8066.

Maybe allowing for new TS_HTTP_* values without breaking compatibility isn't valuable enough to justify the gap though. Can you explain what the problem is with having the gap?

Copy link
Member

@SolidWallOfCode SolidWallOfCode Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #9480 . This is the root of the performance win from that PR, which was discussed at the last PR review. In essence the gap creates a large number of elements in the area that get checked for being cleared but are never used.

Copy link
Contributor

@bneradt bneradt Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

Can both the concerns be addressed by making the TS_SSL_* values a separate enum?

Copy link
Contributor

@bneradt bneradt Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or make the gap smaller? We only count to 20 now. Maybe set the TS_SSL_FIRST_HOOK to 30?

Copy link
Contributor Author

@zwoop zwoop Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the patch does, it makes the gap smaller by not fixing it at 201.

Copy link
Contributor

@bneradt bneradt Mar 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this on slack some. The tradeoff is this:

  1. With a gap, we can add TS_HTTP_* values (hooks) without breaking compatibility.
  2. Without a gap, any new hook breaks compatibility and thus must be done only on major releases.

If the decision is that we only add these hooks on major release, then we don't need a gap in these values. I'm fine with that. I just want to make sure that's understood.

Thus we can:

  1. Remove the gap (as your original patch does) with the understanding that hooks can only be add on major releases.
  2. Make the gap a smaller value to avoid the performance issue, but still keep the gap to allow for new hooks. Maybe leave a space of 5 values.
  3. Refactor the enum into two enums so that the TS_SSL* hooks are in a separate enum. But note that the comment above these enums says that keeping them together in one enum is intentional.

I'm personally fine with any of these.

Copy link
Contributor

@bneradt bneradt Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion on slack, my vote would be to:

  1. Merge in your PR as is and get rid of the gap.
  2. Look into refactoring the enum to separate out these two types (TS_HTTP and TS_SSL) so that the gap isn't needed. Let's keep that a separate PR if that can be done reasonably cleanly enough.

TS_VCONN_START_HOOK = TS_SSL_FIRST_HOOK,
TS_VCONN_PRE_ACCEPT_HOOK = TS_VCONN_START_HOOK, // Deprecated but compatible for now.
TS_VCONN_CLOSE_HOOK,
Expand Down
2 changes: 1 addition & 1 deletion src/traffic_server/InkAPITest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6636,7 +6636,7 @@ enum ORIG_TSHttpHookID {
ORIG_TS_HTTP_REQUEST_BUFFER_READ_COMPLETE_HOOK,
ORIG_TS_HTTP_RESPONSE_CLIENT_HOOK,
ORIG_TS_HTTP_REQUEST_CLIENT_HOOK,
ORIG_TS_SSL_FIRST_HOOK = 201,
ORIG_TS_SSL_FIRST_HOOK,
ORIG_TS_VCONN_START_HOOK = ORIG_TS_SSL_FIRST_HOOK,
ORIG_TS_VCONN_CLOSE_HOOK,
ORIG_TS_SSL_CLIENT_HELLO_HOOK,
Expand Down