[PATCH] D27152: Merge strings using sharded hash tables.

[Sean Silva via Phabricator via llvm-commits]

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

LGTM with nits.

Although I think that this type of optimization makes sense to do, really debug fission is the best approach for reducing this cost like Rafael said in the other thread. Make sure to check with Rafael that he doesn't feel strongly that we should avoid doing this right now.

My reasoning is that one of the main selling points for LLD is that it is fast. It should be fast an there should be no fine-print (e.g. have to build LLD an uncommon way like PGO or LTO, have to change your workflow to use fission, etc.). It's fine for us to have additional advice for users about how to make their links faster, but people are probably not going to be following that advice the first time they try LLD, and that's the time where they decide to keep using LLD or not.



================
Comment at: ELF/OutputSections.cpp:567
+    DenseMap<CachedHashStringRef, size_t> Map;
+    uint8_t Pad[(64 - sizeof(Map) > 0) ? 64 - sizeof(Map) : 1];
+  } OffsetMapTy;
----------------
I think you can use `alignas` to simplify this. (r287783 seems to be surviving in tree using alignas, so I think we can use it).

Also, I think something like `alignas` is actually needed to get the desired effect in all cases. Currently, OffsetMapTy probably has an actual alignment requirement of `sizeof(void*)`, so an arrangement like:
```
First part of DenseMap1
------- cacheline boundary -------
Second part of DenseMap1
Padding
First part of DenseMap2
------- cacheline boundary -------
Second part of DenseMap2
....
```
is still possible, which will still have false sharing. Doubling the padding to 2x the cacheline size avoids this, but `alignas` is probably simpler.


================
Comment at: ELF/OutputSections.cpp:636
+  // one on single core.
+  if (!Config->Threads || StringRef(getenv("LLD_TEST")) == "1") {
+    Builder.reset(new StringTableBuilder(StringTableBuilder::RAW, Alignment));
----------------
The environment variable is unused (and if you want to use it, the same comment as before about more precise naming applies).


https://reviews.llvm.org/D27152