[PATCH] D59797: [COFF] Reorder fields in Chunk and SectionChunk to reduce their size

Reid Kleckner via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 25 16:40:17 PDT 2019


rnk marked 4 inline comments as done.
rnk added inline comments.


================
Comment at: lld/COFF/Chunks.cpp:47
 
+namespace {
+// This class exists just for the purpose of calculating the expected size of
----------------
ruiu wrote:
> rnk wrote:
> > ruiu wrote:
> > > rnk wrote:
> > > > ruiu wrote:
> > > > > This might be useful but at the same time it looks a bit overly cautious? Perhaps the symbol size is more important but we don't have something like this for them, for example.
> > > > Well, we don't have checks for symbol size yet. :)
> > > > 
> > > > Given that we don't have performance monitoring, I really want people to think hard before they casually add another field to SectionChunk. I wouldn't insist on it if we did, but these types of static_asserts have proven useful in LLVM for preventing size creep.
> > > Then maybe reiterating everything again, how about checking directly with a number like `static_assert(sizeof(Chunk) == 48)`? This should suffice to prevent making the struct larger by accident.
> > It would be wrong for 32-bit. Rather than trying to express it as math on sizeof(void*), I think the struct makes it clearer. And it helps document hidden members like vptr.
> Yeah, but it still feels weird to me to repeat all the members again in a different file just for the purpose of assertion. For 32-bit, we can just set the upper bound like `static_assert(sizeof(Chunk) <= 48)` where 48 is the size of the struct in 64-bit.
Fair enough. Honestly, writing down all the fields in one place helped me identify the profitable reorderings, but we don't need to commit it.


================
Comment at: lld/COFF/Chunks.h:232
+  // Relocations for this section.
   ArrayRef<coff_relocation> Relocs;
 
----------------
I'm not sure we need to store this, actually, it's pretty easy to recompute on demand. At the very least, we could shorten the size to 32-bits. I think ~4 billion is a reasonable implementation limit on relocations.


================
Comment at: lld/COFF/Chunks.h:236
   // the thunk instead of the actual original target Symbol.
   std::vector<Symbol *> RelocTargets;
 
----------------
riccibruno wrote:
> I think that `llvm::SmallVector<Symbol *, 0>` is one pointer smaller, with the drawback that it is limited to `2^32-1` elements.
My plan is to see if I can eliminate it completely, but I wanted to treat it separately once I established that the size of this was important and put a cap on it.


================
Comment at: lld/COFF/Chunks.h:259
   StringRef SectionName;
   std::vector<SectionChunk *> AssocChildren;
 
----------------
This, for example, is typically 2-3 elements long: each comdat .text section will have a an assoc pdata and xdata. A singly linked list could suffice instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59797





More information about the cfe-commits mailing list