[PATCH] D54554: [PDB] Add symbol records in bulk

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In https://reviews.llvm.org/D54554#1299963, @aganea wrote:

> Very nice! You're talking about /DEBUG:GHASH, but your optimisation is also beneficial when GHASH isn't used?


It should. I'd expect to get the same absolute 3 second improvement for non-ghash links, which are currently at around 20s, so as a percentage improvement, it's smaller. I haven't measured, so there could be other cache effects that make the improvement smaller or larger. I was mainly using ghash because it makes the symbol rewriting stand out more in the profile, and it means I can iterate and re-profile more quickly.

I think the improvement is probably less pronounced in debug builds, since they should have fewer small records, which I think mostly come from inlined call sites and their variables.



================
Comment at: lld/COFF/PDB.cpp:1051
       SymsBuffer, [&](CVSymbol Sym) -> llvm::Error {
+        if (Sym.length() % alignOf(CodeViewContainer::Pdb) != 0)
+          NeedsRealignment = true;
----------------
aganea wrote:
> What about:
> ```
> unsigned Aligned = alignTo(Sym.length(), alignOf(CodeViewContainer::Pdb));
> NeedsRealignement |= Aligned != Sym.length();
> RealignedSize += Aligned;
> ```
> 
I like it. I think, at the time, I was thinking, "`NeedsRealignment` is captured by reference, I wouldn't want to unconditionally store to memory every iteration", but that's irrelevant since after profiling I saw that this whole `forEachCodeViewRecord` loop helper template gets inlined.


================
Comment at: lld/COFF/PDB.cpp:1088
+          // copied it to apply relocations.
+          RecordBytes = makeMutableArrayRef(
+              const_cast<uint8_t *>(Sym.data().data()), Sym.length());
----------------
aganea wrote:
> Quick question here - if all symbols are already aligned in source stream, aren't they all already sequentially (linearly) arranged in memory? Couldn't we just set `BulkSymbols` just once in that case, and prevent lines 1332-1138? This already applies for `NeedsRealignement` case.
Not quite. Not all symbols in the .debug$S section get copied to the module dbi stream. See `symbolGoesInModuleStream` for the list of records that don't get copied. In practice, the common ones are `S_GDATA32` and `S_UDT`. I think `S_UDT` can appear locally scoped inside of a function, which means any `.debug$S` section can be discontiguous, but in practice, all `S_UDT` records are packed together at the beginning or end.

My plan was to first measure if aligning the symbols in LLVM, thereby avoiding the copy below, is a big win. If it is, then this up front traversal of all the symbols will often be unnecessary. I was going to try disabling it and setting `NeedsRealignment` to false, and then checking if that's a measurable performance win. If it is, then we really want to specialize for two cases: 1. object file has pre-aligned symbols, 2. object file needs realignment. I can imagine having one loop that does the work, validates that each record is aligned, fails if it isn't, the caller then copies and realigns the rest of the symbols, and restarts the loop from where it failed.


================
Comment at: lld/COFF/PDB.cpp:1133
+          if (Sym.data().data() == BulkSymbols.end()) {
+            BulkSymbols = makeArrayRef(BulkSymbols.data(),
+                                       BulkSymbols.size() + Sym.length());
----------------
aganea wrote:
> In same way we have `ArrayRef<>::drop_front()`, a `extend()` or alike function would be nice:
> ```
> BulkSymbols = BulkSymbols.extend(Sym.length());
> ```
> However - this depends on the answer to comment at line 1088.
I think joining two adjacent ArrayRefs is a pretty corner-case use case. Usually we slice big things up into small pieces. Going the other way around is scary. I'm not sure I want to add an API to encourage it. :)


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.h:95
   std::vector<std::string> SourceFiles;
-  std::vector<codeview::CVSymbol> Symbols;
+  std::vector<ArrayRef<uint8_t>> Symbols;
 
----------------
aganea wrote:
> In a subsequent change, you could count how many symbols subsections are there in a File, and `.reserve` this vector in advance.
True. If we did that, it'd probably best to have LLD manage the std::vector, and then pass ownership of it over to the DbiModuleDescriptorBuilder with std::move.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp:67
 
 void DbiModuleDescriptorBuilder::addSymbol(CVSymbol Symbol) {
+  // Defer to the bulk API. It does the same thing.
----------------
aganea wrote:
> This function should probably be removed in the short term. Other places using it would also benefit from adding symbols in bulk.
Maybe, but I think it's out of scope for this change. yaml2pdb uses this, and there are a handful of other callers that use this pattern:
```
  ObjNameSym ONS(SymbolRecordKind::ObjNameSym);
  Compile3Sym CS(SymbolRecordKind::Compile3Sym);
  EnvBlockSym EBS(SymbolRecordKind::EnvBlockSym);
  ... fill them in
  Mod.addSymbol(codeview::SymbolSerializer::writeOneSymbol(
      ONS, Allocator, CodeViewContainer::Pdb));
  Mod.addSymbol(codeview::SymbolSerializer::writeOneSymbol(
      CS, Allocator, CodeViewContainer::Pdb));
  Mod.addSymbol(codeview::SymbolSerializer::writeOneSymbol(
      EBS, Allocator, CodeViewContainer::Pdb));
```

Perhaps we should have some kind of symbol serializer that encapsulates the pattern I'm using. One of the other things we do a lot of is accumulate global S_PUB32, S_SECTION, and other global symbols. I guess I didn't do it that way because I liked the idea of estimating the amount of memory we need for symbols up front.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/DbiModuleDescriptorBuilder.cpp:73
+void DbiModuleDescriptorBuilder::addSymbolsInBulk(
+    ArrayRef<uint8_t> BulkSymbols) {
+  Symbols.push_back(BulkSymbols);
----------------
aganea wrote:
> Early exit if empty? It looks like the first time you're calling this function from `mergeSymbolRecords()` the collection is empty.
Good idea.


https://reviews.llvm.org/D54554