Page MenuHomePhabricator

[CommandLine] Reduce size of Option class
ClosedPublic

Authored by hintonda on May 17 2019, 5:38 PM.

Details

Summary

Reduce size of Option class from 184 bytes to 136 bytes by
placing more member variables in Bit Field (16 bytes), and
reducing the initial sizes of Categories and Subs to 1 (32 bytes).

Saves about 48k for bin/opt.

Diff Detail

Repository
rL LLVM

Event Timeline

hintonda created this revision.May 17 2019, 5:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 5:38 PM
beanz added a comment.May 17 2019, 7:29 PM

This is pretty cool. Since these are usually allocated as globals, this is saving pages of dirtied memory.

One implementation thought. What if we group the 16-bit fields together and make them uint16_t then use a uint16_t as the bitfield type for the remaining 15 bits. The performance of the 16-bit fields should be better if they are aligned (not that cl::opt perf really matters).

hintonda updated this revision to Diff 200130.May 17 2019, 7:49 PM
  • Change type to multiple uint16_t's and reorder members.
hintonda updated this revision to Diff 200134.May 17 2019, 8:21 PM
  • Remove unneeded bit fields.

This is pretty cool. Since these are usually allocated as globals, this is saving pages of dirtied memory.

One implementation thought. What if we group the 16-bit fields together and make them uint16_t then use a uint16_t as the bitfield type for the remaining 15 bits. The performance of the 16-bit fields should be better if they are aligned (not that cl::opt perf really matters).

I tried to keep the relative order as close to the original as possible, but after rereading your comment, I'm unsure if you wanted me to group the non-bit field unit16_t's together as well. Please let me know...

The main thing is to make sure that if you have proper integer sized members they are properly aligned and packed, which this seems to do well.

llvm/include/llvm/Support/CommandLine.h
278 ↗(On Diff #200134)

If I'm counting right (and math isn't my strong point) we are at 15 bits used in the bitfield. I would add another 1-bit field named "Unused" just to mark out that we have one more bit before the bitfield would grow.

If the bitfield grows, it will result in 7-bytes of padding coming back in, which would be unfortunate.

beanz accepted this revision.May 18 2019, 10:58 AM
This revision is now accepted and ready to land.May 18 2019, 10:58 AM
beanz added inline comments.May 18 2019, 11:04 AM
llvm/include/llvm/Support/CommandLine.h
288 ↗(On Diff #200134)

Side note: I wonder how many members actually get put into this. The common-case for most options is surely 0.

beanz added inline comments.May 18 2019, 11:16 AM
llvm/include/llvm/Support/CommandLine.h
288 ↗(On Diff #200134)

From eyeballing (not an exhaustive search), I don't see anywhere that more than one Sub command is set. You might be able to save more space by shrinking the inline size here. If I read the code right, SmallPtrSet is 32 bytes + (sizeof (T) * N). In this case that should be 64 bytes. Changing the size to 2, would save another 16 bytes per option.

hintonda marked an inline comment as done.May 18 2019, 11:58 AM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
288 ↗(On Diff #200134)

Actually, it's empty about 99% or the time and defaults to TopLevelSubCommand at that point. This is how all the options in passes are setup -- well over 1,000. Only a few tools currently use SubCommands -- the most I've seen is 4 by llvm-pdbutil.

So, I was considering something a bit more radical. Instead of each Option keeping track of SubCommand, and Category, membership, why not let the SubCommand's and Category's do it instead?

hintonda marked an inline comment as done.May 18 2019, 12:18 PM
hintonda added inline comments.
llvm/include/llvm/Support/CommandLine.h
288 ↗(On Diff #200134)

So as a quick test, I set both to 1 and reduced the overall size down to 136 bytes -- Categories always has at least 1, but rarely more, and Subs is almost always empty:

--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -283,9 +283,9 @@ public:
   StringRef ArgStr;   // The argument string itself (ex: "help", "o")
   StringRef HelpStr;  // The descriptive text message for -help
   StringRef ValueStr; // String describing what the value of this option is
-  SmallVector<OptionCategory *, 2>
+  SmallVector<OptionCategory *, 1>
       Categories;                    // The Categories this option belongs to
-  SmallPtrSet<SubCommand *, 4> Subs; // The subcommands this option belongs to.
+  SmallPtrSet<SubCommand *, 1> Subs; // The subcommands this option belongs to.
hintonda retitled this revision from [CommandLine] Reduce size of Option class by moving more members into bit field to [CommandLine] Reduce size of Option class.May 18 2019, 12:38 PM
hintonda edited the summary of this revision. (Show Details)
hintonda edited the summary of this revision. (Show Details)
hintonda updated this revision to Diff 200155.May 18 2019, 12:41 PM
  • Reduce initial vector/set size to the expected value.
Closed by commit rL361107: [CommandLine] Reduce size of Option class (authored by dhinton, committed by ). · Explain WhyMay 18 2019, 1:44 PM
This revision was automatically updated to reflect the committed changes.