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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

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



================
Comment at: lld/COFF/PDB.cpp:1051
       SymsBuffer, [&](CVSymbol Sym) -> llvm::Error {
+        if (Sym.length() % alignOf(CodeViewContainer::Pdb) != 0)
+          NeedsRealignment = true;
----------------
What about:
```
unsigned Aligned = alignTo(Sym.length(), alignOf(CodeViewContainer::Pdb));
NeedsRealignement |= Aligned != Sym.length();
RealignedSize += Aligned;
```



================
Comment at: lld/COFF/PDB.cpp:1088
+          // copied it to apply relocations.
+          RecordBytes = makeMutableArrayRef(
+              const_cast<uint8_t *>(Sym.data().data()), Sym.length());
----------------
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.


================
Comment at: lld/COFF/PDB.cpp:1133
+          if (Sym.data().data() == BulkSymbols.end()) {
+            BulkSymbols = makeArrayRef(BulkSymbols.data(),
+                                       BulkSymbols.size() + Sym.length());
----------------
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.


================
Comment at: lld/COFF/PDB.cpp:1144
+
+  if (!BulkSymbols.empty())
+    File->ModuleDBI->addSymbolsInBulk(BulkSymbols);
----------------
Remove `if` if you're early exiting below in `addSymbolsInBulk()`


================
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;
 
----------------
In a subsequent change, you could count how many symbols subsections are there in a File, and `.reserve` this vector in advance.


================
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.
----------------
This function should probably be removed in the short term. Other places using it would also benefit from adding symbols in bulk.


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


https://reviews.llvm.org/D54554