[PATCH] D108850: [LLD] Remove global state in lldCommon
Shoaib Meenai via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Oct 31 18:19:25 PDT 2021
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
More information about the llvm-commits
mailing list