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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lld/ELF/InputSection.h:78
 
+  unsigned : 1;
+
----------------
ruiu wrote:
> pcc wrote:
> > ruiu wrote:
> > > It's not clear to me why you needed a one bit padding here.
> > I added a comment explaining why.
> Why do you have to use a bitfield? Looks like you can use uint8_t instead.
Yeah, that works. For some reason I thought that the series of bitfields would consume sizeof(unsigned) bytes, but that doesn't appear to be the case.


================
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();
----------------
MaskRay wrote:
> 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);
> ```
> You can still have `if (Sec->Partition == 1 || Sec->Partition == Partition)` code in that one single DFS.

It wouldn't help with the problem I mentioned. Here's a concrete example:
```
 A <- root of partition 2
 |
 B <- root of partition 1
/ \
C D
```
If we visit A first we mark A, B, C, D as partition 2. Then when we visit B we visit B, C, D again to mark them as partition 1. But if we visit B first we mark B, C, D as partition 1 and then when we visit A we only mark A as partition 2 and we don't visit B because we'd take the `if (Sec->Partition == 1 || Sec->Partition == Partition)` branch for the `A -> B` edge. So by visiting partitions in order we avoid visiting B, C, D twice by construction.

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

That could easily be addressed with a single pass over the symbol table that groups symbols by partition. I'd still prefer not to do that though, since it seems like a premature optimization that wouldn't be worth the added complexity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60353