[COFF] Pack Name in Symbol as is done in ELF

[rnk]

Summary

This assumes all symbols are <4GB long, so we can store them as a 32-bit
integer. This reorders the fields so the length appears first, packing
with the other bitfield data in the base Symbol object.

This saved 70MB / 3.60% of heap allocations when linking
browser_tests.exe with no PDB. It's not much as a percentage, but worth
doing. I didn't do performance measurements, I don't think it will be
measurable in time.

[ruiu]

lld/COFF/Symbols.h

111 ↗

(On Diff #193813)

I'd use uint32_t to match the comment.

111 ↗

(On Diff #193813)

We have the same hack for ELF, and we name these variables NameSize and NameData.

[aganea]

lld/COFF/Symbols.h

111 ↗

(On Diff #193813)

You could save more by packing this with the bitfield above. ‘SymbolKind’ doesn’t need more than 4 bits, and that would free 21 bits for ‘NameLen’. In practical terms, do we really expect symbol lengths > 2^21 chars? Worst case, there could be a slower/higher mem. codepath for off-the-limits case?

[aganea]

lld/COFF/Symbols.h

111 ↗

(On Diff #193813)

Forget what I said, it’ll be aligned on 64-bits because of the pointer that follows.

[rnk]

• update naming

[thakis]

lld/COFF/Symbols.h

111 ↗

(On Diff #193813)

If uint32_t happens to be a long, will this prevent packing with the bitfields in the ms abi? maybe unsigned is better for that reason?

[rnk]

lld/COFF/Symbols.h

111 ↗

(On Diff #193813)

Yes, and I think that's part of why I used unsigned in the first place. However, right now it's not a bitfield so it won't matter. If we do ever shrink NameSize even further with bitfields, then yes, we'd want to update them all to some common type.

[ruiu]

LGTM

lld/trunk/COFF/Symbols.h

public:
   // after calling markLive.
   bool isLive() const;
 
 protected:
   friend SymbolTable;
   explicit Symbol(Kind K, StringRef N = "")
       : SymbolKind(K), IsExternal(true), IsCOMDAT(false),
         WrittenToSymtab(false), PendingArchiveLoad(false), IsGCRoot(false),
-        IsRuntimePseudoReloc(false), Name(N) {}
+        IsRuntimePseudoReloc(false), NameSize(N.size()),
+        NameData(N.empty() ? nullptr : N.data()) {}
 
   const unsigned SymbolKind : 8;
   unsigned IsExternal : 1;
 
   // This bit is used by the \c DefinedRegular subclass.
   unsigned IsCOMDAT : 1;
 
 public:
public:
   unsigned PendingArchiveLoad : 1;
 
   /// True if we've already added this symbol to the list of GC roots.
   unsigned IsGCRoot : 1;
 
   unsigned IsRuntimePseudoReloc : 1;
 
 protected:
-  StringRef Name;
+  // Symbol name length. Assume symbol lengths fit in a 32-bit integer.
+  uint32_t NameSize;
+
+  const char *NameData;
 };
 
 // The base class for any defined symbols, including absolute symbols,
 // etc.
 class Defined : public Symbol {
 public:
   Defined(Kind K, StringRef N) : Symbol(K, N) {}
 
   static bool classof(const Symbol *S) { return S->kind() <= LastDefinedKind; }
 
   // Returns the RVA (relative virtual address) of this symbol. The
   // writer sets and uses RVAs.
   uint64_t getRVA();
 
   // Returns the chunk containing this symbol. Absolute symbols and __ImageBase
   // do not have chunks, so this may return null.
   Chunk *getChunk();
 };
 
 // Symbols defined via a COFF object file or bitcode file.  For COFF files, this
 // stores a coff_symbol_generic*, and names of internal symbols are lazily
 // loaded through that. For bitcode files, Sym is nullptr and the name is stored
-// as a StringRef.
+// as a decomposed StringRef.
 class DefinedCOFF : public Defined {
   friend Symbol;
 
 public:
   DefinedCOFF(Kind K, InputFile *F, StringRef N, const coff_symbol_generic *S)
       : Defined(K, N), File(F), Sym(S) {}
 
   static bool classof(const Symbol *S) {

lld/trunk/COFF/Symbols.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 using namespace llvm::object;
 
 using namespace lld::coff;
 
+static_assert(sizeof(SymbolUnion) <= 48,
+              "symbols should be optimized for memory usage");
+
 // Returns a symbol name for an error message.
 std::string lld::toString(coff::Symbol &B) {
   if (Config->Demangle)
     if (Optional<std::string> S = lld::demangleMSVC(B.getName()))
       return *S;
   return B.getName();
 }
 
 namespace lld {
 namespace coff {
 
 StringRef Symbol::getName() {
   // COFF symbol names are read lazily for a performance reason.
   // Non-external symbol names are never used by the linker except for logging
   // or debugging. Their internal references are resolved not by name but by
   // symbol index. And because they are not external, no one can refer them by
   // name. Object files contain lots of non-external symbols, and creating
   // StringRefs for them (which involves lots of strlen() on the string table)
   // is a waste of time.
-  if (Name.empty()) {
+  if (NameData == nullptr) {
     auto *D = cast<DefinedCOFF>(this);
-    cast<ObjFile>(D->File)->getCOFFObj()->getSymbolName(D->Sym, Name);
+    StringRef NameStr;
+    cast<ObjFile>(D->File)->getCOFFObj()->getSymbolName(D->Sym, NameStr);
+    NameData = NameStr.data();
+    NameSize = NameStr.size();
+    assert(NameSize == NameStr.size() && "name length truncated");
   }
-  return Name;
+  return StringRef(NameData, NameSize);
 }
 
 InputFile *Symbol::getFile() {
   if (auto *Sym = dyn_cast<DefinedCOFF>(this))
     return Sym->File;
   if (auto *Sym = dyn_cast<Lazy>(this))
     return Sym->File;
   return nullptr;
 }
 
 bool Symbol::isLive() const {
   if (auto *R = dyn_cast<DefinedRegular>(this))
     return R->getChunk()->Live;
   if (auto *Imp = dyn_cast<DefinedImportData>(this))
     return Imp->File->Live;
   if (auto *Imp = dyn_cast<DefinedImportThunk>(this))
     return Imp->WrappedSym->File->ThunkLive;
   // Assume any other kind of symbol is live.
   return true;
 }
 
 // MinGW specific.
 void Symbol::replaceKeepingName(Symbol *Other, size_t Size) {
-  StringRef OrigName = Name;
+  StringRef OrigName = getName();
   memcpy(this, Other, Size);
-  Name = OrigName;
+  NameData = OrigName.data();
+  NameSize = OrigName.size();
 }
 
 COFFSymbolRef DefinedCOFF::getCOFFSymbol() {
   size_t SymSize = cast<ObjFile>(File)->getCOFFObj()->getSymbolTableEntrySize();
   if (SymSize == sizeof(coff_symbol16))
     return COFFSymbolRef(reinterpret_cast<const coff_symbol16 *>(Sym));
   assert(SymSize == sizeof(coff_symbol32));
   return COFFSymbolRef(reinterpret_cast<const coff_symbol32 *>(Sym));