Reduce the size of the APIHooks, eliminating enum gap #9509
Conversation
This is an ABI incompatible change.
zwoop
force-pushed
the
FixAPIHooksOversizing
branch
from
f07e062 to
6b8ba2a
Compare
| // 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see.
Can both the concerns be addressed by making the TS_SSL_* values a separate enum?
There was a problem hiding this comment.
Or make the gap smaller? We only count to 20 now. Maybe set the TS_SSL_FIRST_HOOK to 30?
There was a problem hiding this comment.
That's what the patch does, it makes the gap smaller by not fixing it at 201.
There was a problem hiding this comment.
We talked about this on slack some. The tradeoff is this:
- With a gap, we can add
TS_HTTP_*values (hooks) without breaking compatibility. - 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:
- Remove the gap (as your original patch does) with the understanding that hooks can only be add on major releases.
- 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.
- 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.
There was a problem hiding this comment.
After some discussion on slack, my vote would be to:
- Merge in your PR as is and get rid of the gap.
- Look into refactoring the enum to separate out these two types (
TS_HTTPandTS_SSL) so that the gap isn't needed. Let's keep that a separate PR if that can be done reasonably cleanly enough.
| // 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, |
There was a problem hiding this comment.
After some discussion on slack, my vote would be to:
- Merge in your PR as is and get rid of the gap.
- Look into refactoring the enum to separate out these two types (
TS_HTTPandTS_SSL) so that the gap isn't needed. Let's keep that a separate PR if that can be done reasonably cleanly enough.
This is an ABI incompatible change.
* commit 'c54a2e2b77151869ff014fbdc4c82cec0afcbb8c': (37 commits) Slight performance improvements before calling APIHooks::clear (apache#9480) libswoc: Update to 1.4.5 (apache#9522) CryptoContext: Clean up to avoid compiler problem. (apache#9521) Add TLSCertSwitchSupport (apache#9322) Add clang-format-tests to clang-format target (apache#9456) Adds the AR env variable to config.nice (apache#9515) Fix .asf.yaml (apache#9519) Hugepage config cleanup (apache#9479) Separate io_uring into a separate library. AIO in io_uring mode uses new io_uring lib. (apache#9462) Avoid memory allocation in CryptoHash (apache#9474) UnitParser: add unit parser support. (apache#9485) autest - Minor fix on the verifier_client test ext to allow setting only the http3 ports. (apache#9517) Remove support for port event polling (apache#9476) QUIC: Add support to configure UDP max payload limit. (apache#9486) Reduce the size of the APIHooks, eliminating enum gap (apache#9509) Add support for CMCD-Request header nor field to prefetch plugin (apache#9232) Eliminates padding from some common structs (apache#9481) Enable external file loading for sni.yaml. (apache#9501) Remove inactive include of IpMapConf.h (apache#9512) Cleanup: Remove RecModeT from the code. (apache#9487) ...
This is for apache#8066, apache#8804, apache#9509 and apache#11432.
4 participants
This is an ABI incompatible change.