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

Move trace_object() to SFT #110

Closed
qinsoon opened this issue Jun 24, 2020 · 6 comments · Fixed by #542
Closed

Move trace_object() to SFT #110

qinsoon opened this issue Jun 24, 2020 · 6 comments · Fixed by #542
Labels
A-mutator Area: Mutator C-enhancement Category: Enhancement G-performance Goal: Performance

Comments

@qinsoon
Copy link
Member

qinsoon commented Jun 24, 2020

As raised in #108 (comment), invoking initialize_header() from a Space trait object would result in dynamic dispatching in the fast path. We can move initialize_header() to SFT, this would solve the problem.

@qinsoon qinsoon added C-enhancement Category: Enhancement G-performance Goal: Performance A-mutator Area: Mutator labels Jun 24, 2020
@qinsoon
Copy link
Member Author

qinsoon commented Nov 4, 2020

We want to do a similar refactoring for trace_object() as well.

@qinsoon qinsoon added this to the v0.2 milestone Nov 4, 2020
@qinsoon qinsoon removed this from the v0.2 milestone Nov 9, 2020
@qinsoon
Copy link
Member Author

qinsoon commented Dec 18, 2020

initialize_header() is in SFT. However, we cannot easily move trace_object() to SFT without further refactoring. SFT is a trait object, which means its methods need to be simple. However, trace_object() is a generic method (with T: TransitiveClosure), which is not simple. We need refactoring.

@qinsoon qinsoon changed the title Move initialize_header() to SFT Move trace_object() to SFT Dec 18, 2020
@steveblackburn
Copy link
Contributor

I'd like to up the priority on this. Perhaps this can be done as part of the next bugfix/cleanup release.

@qinsoon
Copy link
Member Author

qinsoon commented Oct 29, 2021

initialize_header() is in SFT. However, we cannot easily move trace_object() to SFT without further refactoring. SFT is a trait object, which means its methods need to be simple. However, trace_object() is a generic method (with T: TransitiveClosure), which is not simple. We need refactoring.

trace_object() uses TransitiveClosure and CopyContext as type parameters. However, once we remove the plan-specific process edges (instead use a general SFT-based process edges), the type TransitiveClosure can be removed. The type parameter for CopyContext doesn't really matter. We can simply use dyn CopyContext - as long as we need to use CopyContext (we are copying which is slow), the overhead of dynamic dispatch on dyn CopyContext shouldn't affect much.

@wks
Copy link
Collaborator

wks commented Oct 29, 2021

trace_object() has two type parameters, but the types themselves are never explicitly used in the function body. So they can be replaced with impl TransitiveClosure and impl CopyContext. They are semantically equivalent, but less verbose. impl SomeTrait means the concrete type of the parameter can still be inferred at compile time, but we just don't mention the concrete type name. Therefore it still has the performance advantage (including opportunities to inline) of generic parameters.

diff --git a/src/policy/copyspace.rs b/src/policy/copyspace.rs
index 77e75321..d6ad9154 100644
--- a/src/policy/copyspace.rs
+++ b/src/policy/copyspace.rs
@@ -182,12 +182,12 @@ impl<VM: VMBinding> CopySpace<VM> {
     }
 
     #[inline]
-    pub fn trace_object<T: TransitiveClosure, C: CopyContext>(
+    pub fn trace_object(
         &self,
-        trace: &mut T,
+        trace: &mut impl TransitiveClosure,
         object: ObjectReference,
         semantics: AllocationSemantics,
-        copy_context: &mut C,
+        copy_context: &mut impl CopyContext,
     ) -> ObjectReference {
         trace!("copyspace.trace_object(, {:?}, {:?})", object, semantics,);
         debug_assert!(

@qinsoon
Copy link
Member Author

qinsoon commented Nov 11, 2021

trace_object() has two type parameters, but the types themselves are never explicitly used in the function body. So they can be replaced with impl TransitiveClosure and impl CopyContext. They are semantically equivalent, but less verbose. impl SomeTrait means the concrete type of the parameter can still be inferred at compile time, but we just don't mention the concrete type name. Therefore it still has the performance advantage (including opportunities to inline) of generic parameters.

diff --git a/src/policy/copyspace.rs b/src/policy/copyspace.rs
index 77e75321..d6ad9154 100644
--- a/src/policy/copyspace.rs
+++ b/src/policy/copyspace.rs
@@ -182,12 +182,12 @@ impl<VM: VMBinding> CopySpace<VM> {
     }
 
     #[inline]
-    pub fn trace_object<T: TransitiveClosure, C: CopyContext>(
+    pub fn trace_object(
         &self,
-        trace: &mut T,
+        trace: &mut impl TransitiveClosure,
         object: ObjectReference,
         semantics: AllocationSemantics,
-        copy_context: &mut C,
+        copy_context: &mut impl CopyContext,
     ) -> ObjectReference {
         trace!("copyspace.trace_object(, {:?}, {:?})", object, semantics,);
         debug_assert!(

I think impl Trait is considered the same as generic type parameters in terms of object safety. That means if we would like to use a trait object, its method cannot use <T> as well as a: impl Trait as parameters.

qinsoon added a commit that referenced this issue Jan 9, 2022
This PR is a step towards #258 and #110. It mainly removes the plan-specific copy context, and replaces with a configurable GC worker copy context that uses policy-specific copy context (copy allocators). The `GCWorkerCopyContext` works similarly to the configurable `Mutator`:
* `GCWorkerCopyContext` ≈ `Mutator`
* `CopySemantics` ≈ `AllocationSemantics`
* `CopyConfig` ≈ `MutatorConfig`

This PR
* renames `CopyContext` (plan-specific) to `PolicyCopyContext`, and moves it to `policy/copy_context`.
  * only copy policies provide implementation for it, namely `CopySpaceCopyContext` and `ImmixCopyContext`.
* adds a `GCWorkerCopyContext` which is a combination of `PolicyCopyContext`, and will be used as the thread local data structure for a GC worker.
  * `CopySemantics` is introduced.
  * A plan needs to provide a `CopyConfig` that maps `CopySemantics` to copy allocators, and map copy allocators to spaces.
  * For copy operation, a proper `CopySemantics` need to be provided.
  * `GCWorkerLocalPtr` and a few type parameters about the `CopyContext` is removed, as now GC worker local and the copy context has the fixed type of `GCWorkerCopyContext`.
* removes the plan specific copy context (such as `SSCopyContext`, `GenCopyCopyContext`, etc)
qinsoon added a commit that referenced this issue Feb 17, 2022
This PR adds a general implementation for `ProcessEdges`, called `SFTProcessEdges`. It uses `SFT.sft_trace_object()` for each policy. A plan does not need to implement their own process edges work packet if they use `SFTProcessEdges`. This PR greatly simplifies the GC work implementation for most GC plans. This PR closes #110, and it is an important step towards #258.

Currently only Immix (including GenImmix) and mark compact uses custom process edges. All the other plans use `SFTProcessEdges`.

This PR
* adds `SFT.sft_trace_object()`.
* adds `Space.set_copy_for_sft_trace()` to set copy context for each space, which is used when we invoke `sft_trace_object()`.
* adds `SFTProcessEdges`, and use it for most plans (except immix/genimmix and mark compact).
qinsoon added a commit to qinsoon/mmtk-core that referenced this issue Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mutator Area: Mutator C-enhancement Category: Enhancement G-performance Goal: Performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants