[PATCH] D87805: [PDB] Merge types in parallel when using ghashing

Mon Sep 28 13:18:08 PDT 2020

rnk added a comment.

In D87805#2298905 <https://reviews.llvm.org/D87805#2298905>, @aganea wrote:

> Some figures with this patch (6-core Xeon) -- link times only:

Thanks! For the MSVC configuration, what debug info flags are used for the compile? /Zi or /Z7, and is /Yu used?

---

I'm a bit distracted by other things, so I haven't done more validation on this yet.

================
Comment at: lld/COFF/DebugTypes.cpp:873
+  GloballyHashedType getGHash() const {
+    return TpiSource::instances[getTpiSrcIdx()]->ghashes[getGHashIdx()];
+  }
----------------
aganea wrote:
> rnk wrote:
> > aganea wrote:
> > > I can't help thinking that this smells like Clang's SourceManager index, where all sources all collated into a single index (not a 2D array).
> > > If you did that, it would reduce the size of the cell data to 32-bit, iff we limit ourselves to 2^32 input records.
> > > Am I being too optimistic? ;-)
> > It's an idea, but it's expensive to decompose a SourceLocation into a file id and file offset. However... we could build one giant array of ghashes indexed by this new combined index. This would copy all .debug$H sections, but it could be worth it. This would save a level of indirection during ghash insertion collision resolution, which might be worth a lot. Hm.
> > 
> > Another thing to consider is that MSVC emits many more types than clang. Users mostly use /Zi, which pre-duplicates them, but if they use /Z7, it would probably break this 32-bit limit on the number of input type records. There are already perf issues with lld /debug:ghash + cl /Z7 (extra .debug$T pass), so maybe it's not worth worrying about.
> > This would copy all .debug$H sections
> I am wondering if we couldn't combine all ghashes into a contiguous virtual range of memory (both the pre-existing .debug$H and the locally computed, "owned" ones). The `MapViewOfFile2/3` APIs allow changing the destination `BaseAddress`. There will be some dangling data around .debug$H mappings because the mapping only works on 64K-ranges, but it's maybe better than copying around a few GB worth of .debug$H sections (which also implies duplicating the memory storage for ghashes, because of the file mapping, unless we `munmap` after each copy).
> 
> > There are already perf issues with lld /debug:ghash + cl /Z7 (extra .debug$T pass)
> Like I mentionned in D55585, once ghashes computation is parallelized, it is faster on a 6-core to use `/DEBUG:GHASH` rather than the default `/DEBUG`. Were you thinking of anything else, when you say "there are already perf issues"? We've been using MSVC cl+LLD+D55585 for a long time and the timings of LLD are close to that of Clang+LLD.
Regarding the MapViewOfFile APIs, maybe. But it might be cheaper to copy the memory into huge pages anyway.

Regarding the perf issues with MSVC /Z7, I mean that MSVC /Z7 objects tend to be truly massive, containing many duplicate types. Those massive objects are usually slow to link. MSVC users typically use /Zi or /Yu to pre-deduplicate some of those types. If they were to use /Z7 instead, there might be more than 4 billion input type records, meaning we can't create a single 32-bit input type index space. But, if you have 4 billion input type records, you already have a size problem, and you can fix it by using either /Zi or clang-cl, which emits less type info.

================
Comment at: lld/COFF/DebugTypes.cpp:903
+
+  // FIXME: The low bytes of SHA1 have low entropy for short records, which
+  // type records are. Swap the byte order for better entropy. A better ghash
----------------
aganea wrote:
> Note for future developement: It would be nice to support other kinds of hashers in `.debug$H`. SHA1 is not the best choice, see https://reviews.llvm.org/D55585#1356894 - xxHash64 seems like a better solution. I've also tried MeowHash, and since it uses AES instructions it run pretty much at memory bandwidth speed: https://github.com/cmuratori/meow_hash
I suppose the way to do this would be to receive GloballyHashedType as a template parameter. Probably necessary, but I worked so hard to make this code untemplated. :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87805