[PATCH] D110018: [lld-macho] Speed up markLive()

[Vy Nguyen via Phabricator via llvm-commits]

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