[PATCH] D122922: [lld][common][lld-macho][lld-coff] Support per-thread allocators and StringSavers

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a subscriber: lattner.
aganea added a comment.

Hello! Thanks for adding me :-) Interesting challenge!
It's a bit sad that we have to do all this high-level/application-level memory management. Most modern allocators already support (lock-free) per-thread memory pools out-of-the-box, along with migration between threads and to the global pool/arena. This patch seems to be needed solely because we use `BumpPtrAllocator/SpecificBumpPtrAllocator`. I wonder how things would perform with just using `malloc()` instead + a modern underlying allocator (rpmalloc, mimalloc, ...). Memory locality brought by the bumpalloc is important, but it'd be interesting to compare benchmarks. FWIW there were discussions with @lattner at some point about integrating `rpmalloc` into the LLVM codebase, but I never got to post a RFC.



================
Comment at: lld/Common/Memory.cpp:19
+// Pointer to this thread's context.
+thread_local PerThreadContext *threadContext = nullptr;
+} // namespace
----------------
As things stand, you could also do `LLVM_THREAD_LOCAL PerThreadContext *threadContext = nullptr;`
However since this is really just like a `static` it goes against the "*LinkerContext" concept, and since you already added some functionality in the context, you could just as well move this in `CommonLinkerContext`. You could do something like `ThreadLocal AllocContext;` (see llvm/include/llvm/Support/ThreadLocal.h). That would allocate a dynamic TLS slot on the first use, as opposed to `thread_local` which points to a pre-allocated slot in the PE static TLS table.


================
Comment at: lld/include/lld/Common/CommonLinkerContext.h:36
+// unnecessary nesting of the mutexes
+struct PerThreadContext {
+  llvm::BumpPtrAllocator bAlloc;
----------------
I would rather name it considering what the struct does, not how it's used? `AllocationContext` maybe?


================
Comment at: lld/include/lld/Common/CommonLinkerContext.h:49
 
   llvm::BumpPtrAllocator bAlloc;
   llvm::StringSaver saver{bAlloc};
----------------
Why not use your new struct here, even when `LLD_THREAD_SAFE_MEMORY == 0`?


================
Comment at: lld/include/lld/Common/CommonLinkerContext.h:84
+
+inline llvm::StringSaver &saverUnsafe() { return context().saver; }
+
----------------
I agree that those "unsafe" functions might not be needed. The same applies to the two "perThread*" functions above. Can we just go through `saver()` and `bAlloc()`? Do we really want application code to explicitly choose either per-thread allocation pool or the global allocation pool?


================
Comment at: lld/include/lld/Common/CommonLinkerContext.h:97
+inline llvm::BumpPtrAllocator &bAlloc() {
+#if THREAD_SAFE_MEMORY
+  return context().bAlloc;
----------------
`LLD_THREAD_SAFE_MEMORY` perhaps? Is this meant to be configured through cmake?


================
Comment at: lld/include/lld/Common/Memory.h:79
+// This is NOT thread safe.
+template <typename T, typename... U> T *makeUnsafe(U &&...args) {
+  return new (getSpecificAllocSingleton<T>().Allocate())
----------------
Is there a need for "unsafe" or "perthread*"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922