[MachO] Reduce size of Symbol and Defined

[smeenai]

Summary

We can lay out Symbol more optimally to reduce its size from 56 bytes to
48 bytes by eliminating unnecessary padding, and we can lay out Defined
such that its bitfield members are placed in the tail padding of Symbol
(on ABIs which support this), to reduce it from 96 bytes to 80 bytes (8
bytes from the Symbol reduction, and 8 bytes from the tail padding
reuse).

This is perf-neutral for an internal app (results from two different
machines):

           smol-syms       baseline        difference (95% CI)
sys_time   7.430 ± 0.202   7.440 ± 0.193   [  -2.6% ..   +2.9%]
user_time  21.443 ± 0.513  21.206 ± 0.396  [  -3.3% ..   +1.1%]
wall_time  20.453 ± 0.534  20.222 ± 0.488  [  -3.7% ..   +1.5%]
samples    9               8

           smol-syms       baseline        difference (95% CI)
sys_time   3.011 ± 0.050   3.040 ± 0.052   [  -0.4% ..   +2.3%]
user_time  10.416 ± 0.075  10.496 ± 0.091  [  +0.1% ..   +1.4%]
wall_time  12.229 ± 0.144  12.354 ± 0.192  [  -0.1% ..   +2.1%]
samples    14              13

However, on the first machine, it reduces maximum resident set size by
65.9 MB (0.8%, from 7.92 GB to 7.85 GB). On the second machine, it
reduces it by 92 MB (1.4%, from 6.40 GB to 6.31 GB).

[smeenai]

This will cause merge conflicts for D113167. I'm happy to wait until that lands, if @oontvoo prefers.

I'll move the asserts to Symbols.cpp if I get positive responses to https://reviews.llvm.org/D113809#3128636.

[int3]

Neat!

[oontvoo]

In D113813#3128715, @smeenai wrote:

This will cause merge conflicts for D113167. I'm happy to wait until that lands, if @oontvoo prefers.

I'll move the asserts to Symbols.cpp if I get positive responses to https://reviews.llvm.org/D113809#3128636.

Thanks! Does anyone want to LGTM D113167 ? :)

Otherwise, since your patch seems less controversial, feel free to ship it .. I can resolve the conflicts afterwards - didn't seem like a big one (I think)

[smeenai]

In D113813#3128878, @oontvoo wrote:

In D113813#3128715, @smeenai wrote:

This will cause merge conflicts for D113167. I'm happy to wait until that lands, if @oontvoo prefers.

I'll move the asserts to Symbols.cpp if I get positive responses to https://reviews.llvm.org/D113809#3128636.

Thanks! Does anyone want to LGTM D113167 ? :)

Otherwise, since your patch seems less controversial, feel free to ship it .. I can resolve the conflicts afterwards - didn't seem like a big one (I think)

I'm not planning to land mine till Monday, and looks like you have your LGTM, so feel free to commit ahead of me.

lld/MachO/Symbols.h

public:
   uint32_t stubsIndex = UINT32_MAX;
 
   uint32_t symtabIndex = UINT32_MAX;
 
   InputFile *getFile() const { return file; }
 
 protected:
   Symbol(Kind k, StringRefZ name, InputFile *file)
-      : symbolKind(k), nameData(name.data), nameSize(name.size), file(file),
+      : symbolKind(k), nameData(name.data), file(file), nameSize(name.size),
         isUsedInRegularObj(!file || isa<ObjFile>(file)),
         used(!config->deadStrip) {}
 
   Kind symbolKind;
   const char *nameData;
-  mutable uint32_t nameSize;
   InputFile *file;
+  mutable uint32_t nameSize;
 
 public:
   // True if this symbol was referenced by a regular (non-bitcode) object.
   bool isUsedInRegularObj : 1;
 
   // True if an undefined or dylib symbol is used from a live section.
   bool used : 1;
 };
 
+static_assert(sizeof(void *) != 8 || sizeof(Symbol) == 48,
+              "Try to minimize Symbol's size; we create many instances");
+
 class Defined : public Symbol {
 public:
   Defined(StringRefZ name, InputFile *file, InputSection *isec, uint64_t value,
           uint64_t size, bool isWeakDef, bool isExternal, bool isPrivateExtern,
           bool isThumb, bool isReferencedDynamically, bool noDeadStrip);
 
   bool isWeakDef() const override { return weakDef; }
   bool isExternalWeakDef() const {
     return isWeakDef() && isExternal() && !privateExtern;
   }
   bool isTlv() const override;
 
   bool isExternal() const { return external; }
   bool isAbsolute() const { return isec == nullptr; }
 
   uint64_t getVA() const override;
 
   // Ensure this symbol's pointers to InputSections point to their canonical
   // copies.
   void canonicalize();
 
   static bool classof(const Symbol *s) { return s->kind() == DefinedKind; }
 
-  InputSection *isec;
-  // Contains the offset from the containing subsection. Note that this is
-  // different from nlist::n_value, which is the absolute address of the symbol.
-  uint64_t value;
-  // size is only calculated for regular (non-bitcode) symbols.
-  uint64_t size;
-  ConcatInputSection *compactUnwind = nullptr;
-
+  // Place the bitfields first so that they can get placed in the tail padding
+  // of the parent class, on platforms which support it.
   bool overridesWeakDef : 1;
   // Whether this symbol should appear in the output binary's export trie.
   bool privateExtern : 1;
   // Whether this symbol should appear in the output symbol table.
   bool includeInSymtab : 1;
   // Only relevant when compiling for Thumb-supporting arm32 archs.
   bool thumb : 1;
   // Symbols marked referencedDynamically won't be removed from the output's
   // symbol table by tools like strip. In theory, this could be set on arbitrary
   // symbols in input object files. In practice, it's used solely for the
   // synthetic __mh_execute_header symbol.
   // This is information for the static linker, and it's also written to the
   // output file's symbol table for tools running later (such as `strip`).
   bool referencedDynamically : 1;
   // Set on symbols that should not be removed by dead code stripping.
   // Set for example on `__attribute__((used))` globals, or on some Objective-C
   // metadata. This is information only for the static linker and not written
   // to the output.
   bool noDeadStrip : 1;
 
 private:
   const bool weakDef : 1;
   const bool external : 1;
+
+public:
+  InputSection *isec;
+  // Contains the offset from the containing subsection. Note that this is
+  // different from nlist::n_value, which is the absolute address of the symbol.
+  uint64_t value;
+  // size is only calculated for regular (non-bitcode) symbols.
+  uint64_t size;
+  ConcatInputSection *compactUnwind = nullptr;
 };
 
+// The Microsoft ABI doesn't support using parent class tail padding for child
+// members, hence the _MSC_VER check.
+#if !defined(_MSC_VER)
+static_assert(sizeof(void *) != 8 || sizeof(Defined) == 80,
+              "Try to minimize Defined's size; we create many instances");
+#endif
+
 // This enum does double-duty: as a symbol property, it indicates whether & how
 // a dylib symbol is referenced. As a DylibFile property, it indicates the kind
 // of referenced symbols contained within the file. If there are both weak
 // and strong references to the same file, we will count the file as
 // strongly-referenced.
 enum class RefState : uint8_t { Unreferenced = 0, Weak = 1, Strong = 2 };
 
 class Undefined : public Symbol {

 union SymbolUnion {
   alignas(Defined) char a[sizeof(Defined)];
   alignas(Undefined) char b[sizeof(Undefined)];
   alignas(CommonSymbol) char c[sizeof(CommonSymbol)];
   alignas(DylibSymbol) char d[sizeof(DylibSymbol)];
   alignas(LazySymbol) char e[sizeof(LazySymbol)];
 };
 
+static_assert(sizeof(SymbolUnion) == sizeof(Defined),
+              "Defined should be the largest Symbol kind");
+
 template <typename T, typename... ArgT>
 T *replaceSymbol(Symbol *s, ArgT &&...arg) {
   static_assert(sizeof(T) <= sizeof(SymbolUnion), "SymbolUnion too small");
   static_assert(alignof(T) <= alignof(SymbolUnion),
                 "SymbolUnion not aligned enough");
   assert(static_cast<Symbol *>(static_cast<T *>(nullptr)) == nullptr &&
          "Not a Symbol");
 

lld/MachO/Symbols.cpp

 uint64_t Symbol::getStubVA() const { return in.stubs->getVA(stubsIndex); }
 uint64_t Symbol::getGotVA() const { return in.got->getVA(gotIndex); }
 uint64_t Symbol::getTlvVA() const { return in.tlvPointers->getVA(gotIndex); }
 
 Defined::Defined(StringRefZ name, InputFile *file, InputSection *isec,
                  uint64_t value, uint64_t size, bool isWeakDef, bool isExternal,
                  bool isPrivateExtern, bool isThumb,
                  bool isReferencedDynamically, bool noDeadStrip)
-    : Symbol(DefinedKind, name, file), isec(isec), value(value), size(size),
-      overridesWeakDef(false), privateExtern(isPrivateExtern),
-      includeInSymtab(true), thumb(isThumb),
+    : Symbol(DefinedKind, name, file), overridesWeakDef(false),
+      privateExtern(isPrivateExtern), includeInSymtab(true), thumb(isThumb),
       referencedDynamically(isReferencedDynamically), noDeadStrip(noDeadStrip),
-      weakDef(isWeakDef), external(isExternal) {
+      weakDef(isWeakDef), external(isExternal), isec(isec), value(value),
+      size(size) {
   if (isec) {
     isec->symbols.push_back(this);
     // Maintain sorted order.
     for (auto it = isec->symbols.rbegin(), rend = isec->symbols.rend();
          it != rend; ++it) {
       auto next = std::next(it);
       if (next == rend)
         break;