[PATCH] D94267: [PDB] Defer relocating .debug$S until commit time and parallelize it

[Alexandre Ganea via Phabricator via llvm-commits]

aganea accepted this revision.
aganea added a comment.
This revision is now accepted and ready to land.

LTGM, thanks!

I've checked several of our binaries, compared the PDB symbol stream, and except for extra S_SKIP records, it's all the same.

Some figures:

Large target (400 MB EXE, 2 GB PDB), MSVC 16.8 .OBJs, Unity files:

|        | Link time | Peak commit |
| Before | 23 sec    | 10.3 GB     |
| After  | 20.5 sec  | 8.5 GB      |
|



                                      Summary
  --------------------------------------------------------------------------------
             4848 Input OBJ files (expanded from all cmd-line inputs)
               61 PDB type server dependencies
               40 Precomp OBJ dependencies
        105084352 Input type records
       5672204074 Input type records bytes
          8275330 Merged TPI records
          2939923 Merged IPI records
            58997 Output PDB strings
          8218778 Global symbol records
         24183908 Module symbol records
          2075680 Public symbol records

Same target as above, but compiled with Clang 11 .OBJs:

|        | Link time | Peak commit |
| Before | 16.9 sec  | 6.1 GB      |
| After  | 15.1 sec  | 5 GB        |
|



                                      Summary
  --------------------------------------------------------------------------------
             4844 Input OBJ files (expanded from all cmd-line inputs)
               61 PDB type server dependencies
                0 Precomp OBJ dependencies
         23827009 Input type records
       1366100202 Input type records bytes
          6548058 Merged TPI records
          2588948 Merged IPI records
            58643 Output PDB strings
          4454924 Global symbol records
         42985893 Module symbol records
          1906582 Public symbol records

For smaller targets (like a game retail target), this patch consistently saves 1.3 sec.



================
Comment at: lld/COFF/PDB.cpp:125
+  void analyzeSymbolSubsection(SectionChunk *debugChunk,
+                               uint32_t &moduleStreamSize,
+                               uint32_t &nextRelocIndex,
----------------
s/moduleStreamSize/moduleSymOffset/ to match the definition.


================
Comment at: lld/COFF/PDB.cpp:218
+  /// Includes record realignment.
+  uint32_t moduleStreamSize = 4;
+
----------------
It's a bit strange that '4' means to do nothing in `finish()` but I understand that it makes the logic in `analyzeSymbolSubsection()` less complicated.


================
Comment at: lld/COFF/PDB.cpp:460
   case SymbolKind::S_LPROC32:
+  case SymbolKind::S_GPROC32_ID:
+  case SymbolKind::S_LPROC32_ID:
----------------
Was this a divergence from MSVC link.exe, or was it handled somewhere else before your patch?


================
Comment at: lld/COFF/PDB.cpp:1510
+    // Write ptrEnd for the S_THUNK32.
+    ulittle32_t *pEnd = reinterpret_cast<ulittle32_t *>(
+        const_cast<uint8_t *>(&newSym.data()[8]));
----------------
I can't say I like poking plain memory without going through structured data, it makes the code less reader-friendly. It's a pity we don't have definitions for serialized structures :-( Nothing you can do now I guess.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94267/new/

https://reviews.llvm.org/D94267