[PATCH] D108850: [LLD] Remove global state in lldCommon

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added subscribers: thakis, smeenai.
smeenai added a comment.

Sorry for the slow review from the Mach-O end.

This seems like a pretty clean solution overall. I was concerned that removing global state would mean we have to thread a ton of context through our entire call stack, but switching to function calls instead is easy enough.

I'll measure the performance of this change this week; we care heavily about linking speed because it dominates incremental build speeds (so fast links can be a huge developer experience boost). I'm not expecting any change from the global variable access changes (especially with LTO), but I'm curious if switching the type-specific bump allocators to using a `DenseMap` for lookup will have any measurable effect.

I see a bunch of discussions about thread-locals; was that for an earlier version of this patch, or am I just missing something? (I don't see anything thread-local related in the current version of the patch.)

To make sure I'm understanding this correctly, can the calls to `freeArena` be removed because the destructor for the context will take care of that now?

I'd like @int3 and/or @thakis to take a look on the Mach-O side as well (since they're far more active), and I imagine @MaskRay is the best person for the ELF side.



================
Comment at: lld/COFF/InputFiles.cpp:1069
 void BitcodeFile::parse() {
+  llvm::StringSaver &saver = lld::saver();
   std::vector<std::pair<Symbol *, bool>> comdat(obj->getComdatTable().size());
----------------
What was the motivation for saving to a local variable here? Is it the use of `saver` in the loop below? (It makes sense to me; I just wanted to confirm my understanding.)


================
Comment at: lld/Common/CommonLinkingContext.cpp:22
+
+CommonLinkingContext &lld::commonContext() {
+  assert(lctx);
----------------
It'd be nice if there was a way to put this in the header, so that it could be inlined even in non-LTO builds, but that'd prevent `lctx` from being file-static, which seems undesirable. I imagine LTO or ThinLTO would be happy to inline this (and I'll confirm when I do performance measurements), so it shouldn't matter too much in the end.


================
Comment at: lld/Common/CommonLinkingContext.cpp:23
+CommonLinkingContext &lld::commonContext() {
+  assert(lctx);
+  return *lctx;
----------------
I'm a bit concerned about the potential performance implications of this `assert`, but I'll measure first.


================
Comment at: lld/Common/Memory.cpp:15-17
+llvm::StringSaver &lld::saver() { return context().saver; }
 
+llvm::BumpPtrAllocator &lld::bAlloc() { return context().bAlloc; }
----------------
Can these be put in the header, so that they can be inlined even in non-LTO builds?


================
Comment at: lld/include/lld/Common/Memory.h:31-32
 
 // These two classes are hack to keep track of all
 // SpecificBumpPtrAllocator instances.
 struct SpecificAllocBase {
----------------
This comment should probably be updated? The tracking is happening in the context now.


================
Comment at: lld/include/lld/Common/Memory.h:49
 
+template <class T> int SpecificAlloc<T>::id = 0;
+
----------------
The contents don't matter at all, just the address (which is used as a key for the instances map inside the context), right? Might be helpful to have a comment to that effect, otherwise the introduction of what seems like new global state is confusing.


================
Comment at: lld/include/lld/Common/Memory.h:51-53
 // Use a static local for these singletons so they are only registered if an
 // object of this instance is ever constructed. Otherwise we will create and
 // register ELF allocators for COFF and the reverse.
----------------
This comment needs to be updated (or perhaps just removed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108850