[PATCH] D60353: ELF: Add basic partition data structures and behaviours.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/MarkLive.cpp:352
   // Follow the graph to mark all live sections.
-  MarkLive<ELFT>().run();
+  for (unsigned CurPart = 1; CurPart <= Partitions.size(); ++CurPart)
+    MarkLive<ELFT>(CurPart).run();
----------------
pcc wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > I'm concerned with the performance.
> > > 
> > > Shall we just do one `MarkLive<ELFT>().run();` with all roots and one `MarkLive<ELFT>(1).moveToMain();`?
> > > 
> > > Before, we have npart+1 runs of DFS, now we have just 2 DFS.
> > Or 1, if you can merge `moveToMain()` into `run()`.
> I don't really see how that could lead to better performance, and the current approach is simpler anyway IMHO.
> 
> The important thing isn't how many runs of DFS we do but how many nodes we visit. With that approach we'd still be visiting the same number of nodes, just in a different order. In fact, it could lead to worse performance. Say we happen to visit a root for a loadable partition before visiting the roots of the main partition, and the loadable partition uses mostly the same code as the main partition. So we mark most of the main partition's code as belonging to the loadable partition. Now we move on to the main partition's roots. That will cause us to visit most of the main partition's code again to mark it as being in the main partition. With the current approach we would visit the main partition's code once and we don't visit it again while handling the loadable partition because of the code on lines 197-198. We only visit sections twice if code is used by multiple loadable partitions but not by the main partition, which is something that we'd need to do anyway.
You can still have `if (Sec->Partition == 1 || Sec->Partition == Partition)` code in that one single DFS.

The time complexity of one DFS is not just O(|edges of visited sections |), but O(|symbols| + |edges of visited sections|):

```
  for (Symbol *S : Symtab->getSymbols())
    if (S->includeInDynsym() && S->Partition == Partition)
      markSymbol(S);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60353