[PATCH] D91018: [ELF] Make InputSection smaller

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/ELF/InputSection.h:393
 
+static_assert(sizeof(InputSection) <= 184, "InputSection is too big");
+
----------------
lxfind wrote:
> MaskRay wrote:
> > lxfind wrote:
> > > Curious, what's the purpose of static_assert like this? Will the program behave incorrectly if the size of InputSection is larger than 184?
> > > Asking because with this assertion whoever adding or removing fields from this class or the base class will need to remember updating this static_assert accordingly, which I am not sure is scalable. That doesn't seem to serve the typical purpose of static_assert (which should be that the program behavior depends on it to be true) I think?
> > There are a few other static_assert for objects which are allocated a lot. This is important to prevent accidental regression or memory usage and performance.
> There is a static_assert for SectionPiece, which makes sense since it's small and simple, and it makes sense that we want to make sure the bit trick makes it fall within 16 bytes.
> But I don't think a class as complicated as InputSection falls into that category. InputSection has 3 levels of inheritance hierarchy, with a ton of members. 184 is kind of like a magic number already (you probably have the compiler printed out the size instead of eye-balled it). This doesn't seem to be the right way to catch memory increases of classes.
> If we really want to have some guarantee here, it might make sense to add a static_assert for SectionBase since it's extended by everything else, but imho not a class so far down the inheritance structure.
> Finally, I'm not sure it would serve a real purpose. Whenever anyone is touching these classes, they would just go ahead and modify the static_assert to make it pass since it''s so complicated anyway, which is pure trouble than catching anything because there isn't really a way to "accidentally" regress it. A regression would come from adding fields on purpose.
> Maybe I am missing something here. I see that @espindola added the other assert, what do you think?
More than 90% sections are InputSection and not other SectionBase derivatives. We even have bits in SectionBase which are only used by InputSection. The type we want to optimize for is InputSection, not SectionBase (which is not allocated independently), and InputSection is an apparent memory usage/performance bottleneck so I think static_assert is on topic.

> If we really want to have some guarantee here, it might make sense to add a static_assert for SectionBase since it's extended by everything else, but imho not a class so far down the inheritance structure.

This static_assert is very useful and I felt bad that we had not added it earlier so that D68065 regressed the memory usage by more than 1%.

This is of course not pure trouble - the fields are very stable. You can search for other logs about my optimization on the structure members.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91018/new/

https://reviews.llvm.org/D91018