[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
----------------
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?)


================
Comment at: lld/include/lld/Common/CommonLinkerContext.h:84
+
+inline llvm::StringSaver &saverUnsafe() { return context().saver; }
+
----------------
aganea wrote:
> 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?
@MaskRay : any concern with removing this option? IIRC, you would like this option for performance reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122922