[PATCH] D123879: [LLD] Alternate implementation for "Support per-thread allocators and StringSavers"

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: lld/include/lld/Common/CommonLinkerContext.h:54
+private:
+  llvm::sys::ThreadLocal<AllocContext> contextIndex;
+  // Vector of the TLS context's addresses.
----------------
oontvoo wrote:
> The reason I reverted the revision that used ThreadLocal was because there was a ~1% regression linking chromium observed by @MaskRay .
> (similarly for preserving the options of using non-threadlocal make()/saver() )
> 
> While I would agree with you that the code would so much simpler (like in this patch) to have all ports unconditionally use this new thread-safe allocators, I'm not sure  we should do that at the cost of performance regressions. For Macho, I didn't see any difference either way, but ELF seemed to get slower.
> 
> 
> 
> 
@oontvoo I agree with you, out of the box this patch can cause pref. regressions to existing code. The COFF driver doesn't use that much allocations in multi-threaded code, that's why I don't see differences. Although the regressions are solvable in my view. @MaskRay are you able to pinpoint in which user code the 1% regression is coming from? Can we retrieve the `SpecificAlloc<>` at a higher level in those cases, and then call `SpecificAlloc<>.make()`, to avoid fetching the TLS address on every `make()` call?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123879