This is an archive of the discontinued LLVM Phabricator instance.

[COFF] De-virtualize Chunk and SectionChunk
ClosedPublic

Authored by rnk on May 23 2019, 6:08 PM.

Details

Summary

Shaves another pointer off of SectionChunk, reducing the size from 96 to
88 bytes, down from 144 before I started working on this. Combined with
D62356, this reduced peak memory usage when linking chrome_child.dll
from 713MB to 675MB, or 5%. At this point, I declare that I have reached
diminishing returns.

Create NonSectionChunk to provide virtual dispatch to the rest of the
chunk types.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

rnk created this revision.May 23 2019, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 6:08 PM
rnk updated this revision to Diff 201109.May 23 2019, 6:12 PM
  • fix classof bug
aganea added inline comments.May 24 2019, 9:21 AM
lld/COFF/Chunks.h
355 ↗(On Diff #201109)

This is back to the future! C with manual vtables :) 64-bit pointers are a big problem for space. It'd be nice if we could optionally have 32-bit or even 16-bit pointers (yes it opens the door to other issues, but that could be catched with sanitizers or assertions). In many cases we pool objects of a given type or a given class hierarchy, so they are quite close to each other - having an option to copy the vtable close to the allocated pool would be nice to alleviate for issues like this. A 16-bit index would be more that enough in that case, in place of the 64-bit vtable.

377 ↗(On Diff #201109)

Since you're doing this, I'm just wondering if the behavior for isHotPatchable shouldn't all be here? I find there's value having all related code together. Maybe there are other cases for the other "virtual-ish" functions below.

inline bool Chunk::isHotPatchable() const {
  if (ChunkKind == SectionKind)
    return File->HotPatchable;
  return ChunkKind  == ImportThunk; // add ImportThunk kind
}
rnk marked 2 inline comments as done.May 24 2019, 11:45 AM
rnk added inline comments.
lld/COFF/Chunks.h
355 ↗(On Diff #201109)

Definitely, back to the future. :) I actually did this before for the LLVM IR Value / Instruction class hierarchy:
https://github.com/llvm/llvm-project/blob/308e82ecebeef1342004637db9fdf11567a299b3/llvm/lib/IR/Value.cpp#L98
https://github.com/llvm/llvm-project/blob/308e82ecebeef1342004637db9fdf11567a299b3/llvm/lib/IR/Instruction.cpp#L654
There we have an almost completely closed type hierarchy, so all cases are enumerated in a switch, but here in LLD, some Chunks are defined in files, and we'd first have to give enums to them all. We surely don't need more than 256 Chunk types, so a byte is enough.

377 ↗(On Diff #201109)

I think the way to go is to make this change, and then tackle the remaining virtual methods one by one. This patch replaces the hasData() virtual method with the HasData bit, and there are a number of other methods here that could probably use the same treatment.

Initially I tried to lift section Characteristics up into Chunk in the first version of this patch, but it didn't work out, and I backed it out. I put the P2Align data in to Characteristics and had all the Chunk subtypes simply set their Characteristics in the constructor, but I needed another byte for the ChunkKind, or at least a bit to indicate SectionChunk vs. NonSectionChunk. I couldn't get the data packed in right without doing more unrelated rearrangements, so I left it for later.

aganea accepted this revision.May 24 2019, 12:00 PM
This revision is now accepted and ready to land.May 24 2019, 12:00 PM
This revision was automatically updated to reflect the committed changes.
rnk marked an inline comment as done.May 24 2019, 1:33 PM
rnk added inline comments.
lld/COFF/Chunks.h
377 ↗(On Diff #201109)

This suggested change became D62422.