You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "zwoop (via GitHub)" <gi...@apache.org> on 2023/03/01 20:28:01 UTC
[GitHub] [trafficserver] zwoop opened a new pull request, #9481: Eliminates padding from some common structs
zwoop opened a new pull request, #9481:
URL: https://github.com/apache/trafficserver/pull/9481
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 ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9481: Eliminates padding from some common structs
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9481:
URL: https://github.com/apache/trafficserver/pull/9481#discussion_r1131310692
##########
iocore/eventsystem/I_EventProcessor.h:
##########
@@ -32,7 +32,7 @@
#ifdef TS_MAX_THREADS_IN_EACH_THREAD_TYPE
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;
Review Comment:
Nah ya never know when you'll need 16 bytes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #9481: Eliminates padding from some common structs
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on PR #9481:
URL: https://github.com/apache/trafficserver/pull/9481#issuecomment-1451011791
+1. Great! I've wanted this for a long time! Let's tidy up little by little.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #9481: Eliminates padding from some common structs
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #9481:
URL: https://github.com/apache/trafficserver/pull/9481#issuecomment-1452072645
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` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a diff in pull request #9481: Eliminates padding from some common structs
Posted by "ywkaras (via GitHub)" <gi...@apache.org>.
ywkaras commented on code in PR #9481:
URL: https://github.com/apache/trafficserver/pull/9481#discussion_r1130303391
##########
iocore/eventsystem/I_EventProcessor.h:
##########
@@ -32,7 +32,7 @@
#ifdef TS_MAX_THREADS_IN_EACH_THREAD_TYPE
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;
Review Comment:
This seems pretty rando.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on a diff in pull request #9481: Eliminates padding from some common structs
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #9481:
URL: https://github.com/apache/trafficserver/pull/9481#discussion_r1131219945
##########
iocore/eventsystem/I_EventProcessor.h:
##########
@@ -32,7 +32,7 @@
#ifdef TS_MAX_THREADS_IN_EACH_THREAD_TYPE
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;
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop merged pull request #9481: Eliminates padding from some common structs
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop merged PR #9481:
URL: https://github.com/apache/trafficserver/pull/9481
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org