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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: lld/Common/Memory.cpp:19
+// Pointer to this thread's context.
+thread_local PerThreadContext *threadContext = nullptr;
+} // namespace
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922