[PATCH] D133003: [ELF] Parallelize relocation scanning

[Andrew Ng via Phabricator via llvm-commits]

andrewng added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:1566-1571
   for (Partition &part : partitions) {
     for (EhInputSection *sec : part.ehFrame->sections)
       scanner.template scanSection<ELFT>(*sec);
     if (part.armExidx && part.armExidx->isLive())
       for (InputSection *sec : part.armExidx->exidxSections)
         scanner.template scanSection<ELFT>(*sec);
----------------
MaskRay wrote:
> andrewng wrote:
> > This is running on the main thread. Is there a chance that this might clash with thread 0 of the task pool?
> Thanks for catching this. The main thread doing heavy work will contend with the thread pool. Changed to use `tg.execute`.
I think the previous code could have actually caused a threading issue, i.e. concurrent updates to the `0` indexed relocation vector. This will ensure that can't happen.

The only minor thing is it looks a little odd that the "serial" case uses `tg` but I guess it is still serial.


================
Comment at: lld/ELF/Relocations.cpp:1559
+    };
+    if (serial)
+      fn();
----------------
MaskRay wrote:
> andrewng wrote:
> > MaskRay wrote:
> > > andrewng wrote:
> > > > I wonder if it might be worthwhile using the previous code for the serial case? Although, it probably doesn't make a big difference to performance.
> > > Use which piece of code for the serial case?
> > I was thinking this:
> > ```
> > for (InputSectionBase *sec : inputSections)
> >   if (sec->isLive() && (sec->flags & SHF_ALLOC))
> >     scanner.template scanSection<ELFT>(*sec);
> > ```
> > But on the other hand, in terms of future development and maintenance, it's probably better to use as much of the same code for both "paths", even if there's a minor performance penalty for the serial one.
> Yes, using the same code for both paths is better for maintenance.
Yes, I think I agree. If it only affected the single threaded case, I wouldn't have mentioned it. But as there are specific configurations that are limited to serial I thought that it might be worth considering.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133003