Skip to content

Conversation

@zwoop
Copy link
Contributor

@zwoop zwoop commented Mar 1, 2023

There's still plenty to be done here, but this is pretty tedious work ... And have to be careful too, which I'm not known for ...

@zwoop zwoop added this to the 10.0.0 milestone Mar 1, 2023
@zwoop zwoop requested review from bneradt and masaori335 March 1, 2023 20:27
@zwoop zwoop self-assigned this Mar 1, 2023
@masaori335
Copy link
Contributor

+1. Great! I've wanted this for a long time! Let's tidy up little by little.

@zwoop
Copy link
Contributor Author

zwoop commented Mar 2, 2023

Yeh, we've done this in the past as well, things just creeps up. I think longer term (intern maybe, good idea Craig) would be to make a very consistent layout of all structs and classes (in this order), one section for public and one section for private:

  1. All 64-bit types / pointers // (these will never be padded, so could go last as well
  2. All enum types // These can vary in size, I think, typically 2 bytes maybe ?
  3. All bitfields
  4. All bool types, or other 1 byte types
  5. All 32-bit types

This won't solve all padding issues, but, it at least avoids a lot of them. Our HttpSM.h is particularly messy, I fixed some of it, but it was going back and forth between public / private. I fixed some of that, but not all.

Additionally, it can help to arrange the public vs private sections in a way that the padding between the two is minimized. This may require reordering the list above around those boundaries.

I used a tool call pahole for this. It's definitely not great, because of our class structures and stuff, and doesn't do an awesome job with classes. Maybe there are better tools to investigate, or, maybe just use -Wpadding ?

@bryancall bryancall requested review from bryancall and ywkaras and removed request for bneradt and masaori335 March 6, 2023 23:40
constexpr int MAX_THREADS_IN_EACH_TYPE = TS_MAX_THREADS_IN_EACH_THREAD_TYPE;
#else
constexpr int MAX_THREADS_IN_EACH_TYPE = 3072;
constexpr int MAX_THREADS_IN_EACH_TYPE = 3071;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems pretty rando.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was pretty random, I was going to restore it but ran into a squirrel, but making that odd also made it 16 bytes smaller for some reason ... Should we restore it? It'd be surprising if 3072 is a magic number that actually matters?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah ya never know when you'll need 16 bytes.

@zwoop zwoop merged commit 1578ee9 into apache:master Mar 9, 2023
@zwoop zwoop deleted the Paddington branch March 9, 2023 20:35
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants