[PATCH] D110018: [lld-macho] Speed up markLive()
Vy Nguyen via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 20 09:33:20 PDT 2021
oontvoo marked 2 inline comments as done.
oontvoo added inline comments.
================
Comment at: lld/MachO/InputSection.h:101
public:
+ ConcatInputSection(ConcatInputSection *isec)
+ : ConcatInputSection(isec->shared->segname, isec->shared->name,
----------------
thevinster wrote:
> Why do we need to define our own copy constructor? Isn't the compiler doing that for us already?
because the mutex makes it non-trivially copyable(ie., the default copy ctor won't work).
(This reminds me I should change this to a proper copy ctor :) )
================
Comment at: lld/MachO/InputSection.h:118
// ConcatInputSections are entirely live or dead, so the offset is irrelevant.
- bool isLive(uint64_t off) const override { return live; }
- void markLive(uint64_t off) override { live = true; }
+ bool isLive(uint64_t off) override { return live; }
+ void markLive(uint64_t off) override {
----------------
thevinster wrote:
> Shouldn't this method also have the mutex as well?
yes - missed this one
================
Comment at: lld/MachO/MarkLive.cpp:48
assert(!s->isCoalescedWeak());
+ const std::lock_guard<std::mutex> l(listMutex);
worklist.push_back(s);
----------------
thevinster wrote:
> I think we might have a race condition here. If two different symbols pointing to the same `isec` and `off` happen to enter this function, we could end up writing the same value twice to `worklist`. It feels like the lock should be at the `enqueue` scope instead of the list here.
>
> Doing so, I believe we can also remove all the locks on the `isLive` and `markLive` methods.
No, if the lock were to be put on the whole enqueue() then the whole thing would essentially be single-threaded (because all different sections would have to wait for the same lock).
We should minimise the locked region here.
And yes - this should have checked isLive() again before pushing into the queue
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110018/new/
https://reviews.llvm.org/D110018
More information about the llvm-commits
mailing list