Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use macro to generate process edges for plans #575

Merged
merged 24 commits into from
May 13, 2022

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Apr 27, 2022

This PR adds PlanProcessEdges, and a few macros/traits that are used in PlanProcessEdges. This PR removes policy-specific code from plan, and use macros to generate trace object work for each plan. This PR closes #258, and closes #576.

  • add a trait PolicyTraceObject. Each policy provides an implementation of this. With this trait, a plan no longer include any policy-specific code in trace object.
  • add a trait PlanTraceObject and related macros. With the macro, plan implementer can declaratively specify trace object behavior with attributes, and the implementation of PlanTraceObject will be generated by the macro.
  • add a type PlanProcessEdges. This uses PlanTraceObject to provide a general implementation of ProcessEdgesWork. We no longer need any plan-specific process edges work.

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Apr 27, 2022
macros/trace_object/src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/policy/immix/immixspace.rs Outdated Show resolved Hide resolved
macros/trace_object/src/lib.rs Outdated Show resolved Hide resolved
@qinsoon
Copy link
Member Author

qinsoon commented May 8, 2022

Performance result for this PR. This PR only changes GC. These results show the stop-the-world time. More results can be found in the links at the end. The results are based on d09c58a (with the scan_object() change).

semispace
macro-ss-stw

gen immix
macro-genix-stw

immix
macro-genix-stw

Perf links (not publicly visible)

semispace https://squirrel.anu.edu.au/plotty/yilin/yp-2015/#0|bear-2022-05-05-Thu-100348-ss-macro-process-edges-new&benchmark^build^invocation^iteration&bmtime^time.other^time.stw&|10&iteration^1^2|20&1^invocation|30&1&benchmark&build;jdk-mmtk-master|41&Histogram%20(with%20CI)^build^benchmark&
gen immix https://squirrel.anu.edu.au/plotty/yilin/yp-2015/#0|bear-2022-05-02-Mon-095232-genix-macro-process-edges-new&benchmark^build^invocation^iteration&bmtime^time.other^time.stw&|10&iteration^1^2|20&1^invocation|30&1&benchmark&build;jdk-mmtk-master|41&Histogram%20(with%20CI)^build^benchmark&
immix https://squirrel.anu.edu.au/plotty/yilin/yp-2015/#0|bear-2022-04-28-Thu-152509-immix-macro-process-edges&benchmark^build^invocation^iteration&bmtime^time.other^time.stw&|10&iteration^1^2|20&1^invocation|30&1&benchmark&build;jdk-mmtk-master|41&Histogram%20(with%20CI)^build^benchmark&

wks and others added 3 commits May 9, 2022 09:40
@qinsoon qinsoon marked this pull request as ready for review May 9, 2022 05:16
@qinsoon qinsoon requested review from wenyuzhao and wks May 9, 2022 05:17
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly OK, except some minor spelling, documentation and structural problems.

docs/tutorial/src/mygc/ss/collection.md Outdated Show resolved Hide resolved
docs/tutorial/src/mygc/ss/collection.md Outdated Show resolved Hide resolved
docs/tutorial/src/mygc/ss/collection.md Outdated Show resolved Hide resolved
docs/tutorial/src/mygc/ss/collection.md Outdated Show resolved Hide resolved
docs/tutorial/src/mygc/ss/collection.md Outdated Show resolved Hide resolved
macros/trace_object/src/lib.rs Outdated Show resolved Hide resolved
src/plan/transitive_closure.rs Outdated Show resolved Hide resolved
src/plan/transitive_closure.rs Outdated Show resolved Hide resolved
src/plan/mod.rs Outdated Show resolved Hide resolved
src/scheduler/gc_work.rs Show resolved Hide resolved
Copy link
Member

@wenyuzhao wenyuzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR. It helps refactor a large part of the codebase and makes the code cleaner and shorter. It also removes the dynamic dispatch introduced in SFTProcessEdges.

Here're some of my high-level concerns:

  1. The macro and the unification of trace_object forces all the policies to have the same trace_object signature. But different policies will have different implementations of trace_object with different behaviours and parameters, or even different return values. One immediate drawback of this PR is that if a policy wants to pass one more argument, it has to change the unified interface, which will also change the macro definition code and all the other policies to adapt to the interface change. This adds a cost to future development.
  2. #[derive(PlanTraceObject)] implies that each plan or policy can only be bind to one trace_object implementation. For GC like Immix which requires multiple implementations, it has to introduce another generic args TraceKind. I'm afraid this is probably not a nice and clean approach. TraceKind was introduced only because ImmixSpace has two different ProcessEdges, and they share a large amount of common code. But this is not the case for other policies. For policies with multiple different ProcessEdges and trace_object, introducing TraceKind again in PlanTraceObject will only make things more complicated.
    • In addition, different policies defines different TraceKind values, this makes the meaning of TraceKind become different at different places. This also makes the compiler (or human) harder to check if all the TraceKind value matching and comparison code are written correctly.

src/policy/gc_work.rs Show resolved Hide resolved
macros/trace_object/src/derive_impl.rs Outdated Show resolved Hide resolved
macros/trace_object/src/derive_impl.rs Outdated Show resolved Hide resolved
src/plan/transitive_closure.rs Outdated Show resolved Hide resolved
src/plan/transitive_closure.rs Outdated Show resolved Hide resolved
src/plan/generational/copying/gc_work.rs Show resolved Hide resolved
src/plan/transitive_closure.rs Outdated Show resolved Hide resolved
src/plan/transitive_closure.rs Outdated Show resolved Hide resolved
src/plan/transitive_closure.rs Outdated Show resolved Hide resolved
macros/trace_object/src/util.rs Outdated Show resolved Hide resolved
@qinsoon
Copy link
Member Author

qinsoon commented May 11, 2022

Thanks for the reviews. Before going into each comment, I will just address your concerns first.

Here're some of my high-level concerns:

  1. The macro and the unification of trace_object forces all the policies to have the same trace_object signature. But different policies will have different implementations of trace_object with different behaviours and parameters, or even different return values. One immediate drawback of this PR is that if a policy wants to pass one more argument, it has to change the unified interface, which will also change the macro definition code and all the other policies to adapt to the interface change. This adds a cost to future development.

Do you have an example in your mind about extra parameters in trace_object()? If we need extra parameters, there are two things we can do:

  1. if the extra parameter is general and other policy may need it as well, we can change trace_object() and change the macro.
  2. if the extra parameter is not general and only specific to a policy (first we need a good reason to explain why it is special), one can implement their own trace_object() method, and not use PlanTraceObject/PolicyTraceObject.
  1. #[derive(PlanTraceObject)] implies that each plan or policy can only be bind to one trace_object implementation. For GC like Immix which requires multiple implementations, it has to introduce another generic args TraceKind. I'm afraid this is probably not a nice and clean approach. TraceKind was introduced only because ImmixSpace has two different ProcessEdges, and they share a large amount of common code. But this is not the case for other policies. For policies with multiple different ProcessEdges and trace_object, introducing TraceKind again in PlanTraceObject will only make things more complicated.

    • In addition, different policies defines different TraceKind values, this makes the meaning of TraceKind become different at different places. This also makes the compiler (or human) harder to check if all the TraceKind value matching and comparison code are written correctly.

Depending on TraceKind, trace_object<const K: TraceKind>() are actually specialized into different implementations. Both immix and mark compact have different traces. In my opinion, for example, in immix, calling fast_trace_object() and trace_objecct() has no difference than trace_object<TRACE_KIND_FAST>() and trace_object<TRACE_KIND_DEFRAG>().

For your concern about using u8 for TraceKind, it is less ideal. But it can be mitigated that each policy will only check their known trace kind, and panic for unknown traces.

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably have only one macro crate called mmtk-macros rather than multiple macro crate for different features, such as mmtk-macro-trace-object.

We may have more macros in the future. Then,

  1. Different macros may share common utilities, such as handling attributes.
  2. The sub crates that contain macros will also appear on crates.io

The second point matters. If we have multiple crates, then there will be multiple mmtk-macro-xxx crates on crates.io. They will be visible to the world as:

And they need to be versioned, too. If we add more features, we will introduce even more crates. Although having multiple crates doesn't seem to violate crates.io's policy (They do, however, disallow "using an automated tool to claim ownership of a large number of package names".), it is much harder to maintain so many crates.

macros/trace_object/Cargo.toml Outdated Show resolved Hide resolved
macros/trace_object/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me now. Please make sure Wenyu's comments are also addressed.

Copy link
Member

@wenyuzhao wenyuzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
3 participants