This is an archive of the discontinued LLVM Phabricator instance.

Use a class instead of lambda-based callbacks to organize garbage collector.
ClosedPublic

Authored by ruiu on Mar 25 2019, 3:01 PM.

Details

Summary

lld's mark-sweep garbage collector was written in the visitor pattern.
There are functions that traverses a given graph, and the functions calls
callback functions to dispatch according to node type.

The code was originaly pretty simple, and lambdas worked pretty
well. However, as we add more features to the garbage collector, that became
more like a callback hell. We now have a callback function that wraps
another callback function, for example. It is not easy to follow the flow of
the control.

This patch rewrites it as a regular class. What was once a lambda is now a
regular class member function. I think this change fixes the readability
issue.

No functionality change intended.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

ruiu created this revision.Mar 25 2019, 3:01 PM
Herald added a project: Restricted Project. · View Herald Transcript
ruiu updated this revision to Diff 192212.Mar 25 2019, 3:11 PM
  • minor tweaks
pcc added inline comments.Mar 25 2019, 3:28 PM
lld/ELF/MarkLive.cpp
255 ↗(On Diff #192211)

This seems fine for now, but with partitioning we'll need to do this multiple times:
https://github.com/pcc/llvm-project/blob/d40cc2e48b560445c4c741cc72a8c6bab47fb2eb/lld/ELF/MarkLive.cpp#L269
https://github.com/pcc/llvm-project/blob/d40cc2e48b560445c4c741cc72a8c6bab47fb2eb/lld/ELF/MarkLive.cpp#L288

Maybe it should be the user of MarkLive who adds GC roots? Then with partitioning the code will end up looking like:

for (unsigned I = 1; I != NumPartitions; ++I) {
  MarkLive<ELFT> M(I);
  // enqueue symbols/sections for partition I
  M.mark();
}

where mark() contains the code in this loop.

ruiu marked an inline comment as done.Mar 25 2019, 3:35 PM
ruiu added inline comments.
lld/ELF/MarkLive.cpp
255 ↗(On Diff #192211)

Yeah maybe. This change is more like a mechanical translation with some inlining, so I don't know if I should do that now. Do you want me to modify this change?

pcc accepted this revision.Mar 25 2019, 4:05 PM

LGTM

lld/ELF/MarkLive.cpp
255 ↗(On Diff #192211)

No need to modify it. Once I start sending the partitioning changes for review I'll figure out what to do here.

This revision is now accepted and ready to land.Mar 25 2019, 4:05 PM
This revision was automatically updated to reflect the committed changes.