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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/Common/Memory.cpp:19
+// Pointer to this thread's context.
+thread_local PerThreadContext *threadContext = nullptr;
+} // namespace
----------------
oontvoo wrote:
> MaskRay wrote:
> > oontvoo wrote:
> > > aganea wrote:
> > > > aganea wrote:
> > > > > oontvoo wrote:
> > > > > > aganea wrote:
> > > > > > > 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.
> > > > > > thanks! I've moved the variable to the LinkerContext class but wasn't able to use LLVM's ThreadLocal because I needed to hold on to the variable's address (to reset its value from another thread).
> > > > > > 
> > > > > > Would this "just work" with ThreadLocal?
> > > > > > 
> > > > > > ie:
> > > > > > ```
> > > > > > ThreadLocal<AllocationContext> currentThreadCtxt;
> > > > > > 
> > > > > > 
> > > > > > std::vector<ThreadLocal<AllocationContext>> allThreadContexts;
> > > > > > 
> > > > > > // collecting all the thread-local ctxt's addresses
> > > > > > allThreadContext.push_back(&currentThreadCtxt);
> > > > > > 
> > > > > > // in destroy(), reset them to sentinel value (nullptr)
> > > > > > for (auto& localCtxt : allThreadContexts) {
> > > > > >   delete localCtxt.get();
> > > > > >   localCtxt = nullptr;
> > > > > > }
> > > > > > 
> > > > > > ```
> > > > > > 
> > > > > It is not possible to take the address of the TLS slot -- one can only `.get()` or `.set()` the pointer in the `ThreadLocal` object. We needed that address to reset to sentinel in the destructor. But if moving `ThreadLocal<AllocationContext>` into `CommonLinkerContext`, we don't need to reset to a sentinel value anymore, since the TLS slot in all threads will die with `CommonLinkerContext`. If the TLS slot is reused later, it'll be reset to 0 by the system. So we could just use `std::vector<AllocationContext *> allThreadContexts` (plain pointer).
> > > > > ```
> > > > > struct CommonLinkerContext {
> > > > >   ThreadLocal<AllocationContext> currentThreadCtxt;
> > > > >   std::vector<AllocationContext *> allThreadContexts;
> > > > >   ...
> > > > > };
> > > > > 
> > > > > AllocationContext *CommonLinkerContext::allocCtxt() {
> > > > >   if (!threadContext.get()) {
> > > > >     // Context didn't exist yet for this thread, so create a new one.
> > > > >     auto *context = new AllocationContext;
> > > > >     threadContext.set(context);
> > > > > 
> > > > >     llvm::sys::ScopedWriter lock(contextMutex);
> > > > >     allThreadContexts.push_back(context);
> > > > >   }
> > > > >   return threadContext.get();
> > > > > }
> > > > > 
> > > > > CommonLinkerContext::~CommonLinkerContext() {
> > > > >   ...
> > > > >   llvm::sys::ScopedWriter lock(contextMutex);
> > > > >   for (AllocationContext *context : allThreadContexts) {
> > > > >     for (auto &instance : context->instances)
> > > > >       instance.second->~SpecificAllocBase();
> > > > >     delete context;
> > > > >   }
> > > > >   ...
> > > > > }
> > > > > ```
> > > > There's still a chance that an `AllocationContext` is in use when calling `~CommonLinkerContext()`, if all threads were not `.join`ed prior. But that should probably be solved at a higher level?
> > > > There's still a chance that an AllocationContext is in use when calling ~CommonLinkerContext(), if all threads were not .joined prior. But that should probably be solved at a higher level?
> > > 
> > > Yes, it's already a problem now with the saver()'s and bAlloc() 's return objects being used after the context has already been destroyed.
> > > (We can rely on asan/msan for catching this, no?)
> > We can use LLVM_THREAD_LOCAL instead of llvm/Support/ThreadLocal.h. llvm/Support/ThreadLocal.h uses pthread_getspecific which is slower than TLS.
> > 
> > LLVM_THREAD_LOCAL has been used in llvm/lib/Support and clang.
> > 
> > 
> yeah, I've noted  that pthread_getspecific could be a bit problematic but in practice I didn't see any performance impact when running the benchmarks.
> The advantage of using the ThreadLocal class is that we could put the "tag" inside the context class and that's better encapsulation.
> WDYT?
Well, I observed a close to 1% regression when linking chromium :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922