-
Notifications
You must be signed in to change notification settings - Fork 69
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
Allow binding to implement GC trigger #1083
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if self.policy.is_gc_required(space_full, space, plan) { | ||
if self | ||
.policy | ||
.is_gc_required(space_full, space.map(|s| SpaceStats::new(s)), plan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR: Why is Plan
safe to be exposed to the binding, whereas Space
is not? (See L126 below in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's still possible to access spaces from a plan reference?
Lines 142 to 155 in 7081e86
pub trait Plan: 'static + HasSpaces + Sync + Downcast { | |
/// Get the plan constraints for the plan. | |
/// This returns a non-constant value. A constant value can be found in each plan's module if needed. | |
fn constraints(&self) -> &'static PlanConstraints; | |
/// Create a copy config for this plan. A copying GC plan MUST override this method, | |
/// and provide a valid config. | |
fn create_copy_config(&'static self) -> CopyConfig<Self::VM> { | |
// Use the empty default copy config for non copying GC. | |
CopyConfig::default() | |
} | |
/// Get a immutable reference to the base plan. `BasePlan` is included by all the MMTk GC plans. | |
fn base(&self) -> &BasePlan<Self::VM>; |
Lines 326 to 334 in 7081e86
pub struct BasePlan<VM: VMBinding> { | |
pub(crate) global_state: Arc<GlobalState>, | |
pub options: Arc<Options>, | |
pub gc_trigger: Arc<GCTrigger<VM>>, | |
// Spaces in base plan | |
#[cfg(feature = "code_space")] | |
#[space] | |
pub code_space: ImmortalSpace<VM>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR: Why is
Plan
safe to be exposed to the binding, whereasSpace
is not? (See L126 below in this file)
Space
is not public before this PR, and I don't think we have a good reason to expose it. So I decided to wrap Space
into a public type SpaceStats
.
For Plan
, I don't think we want to expose Plan
either (at least some methods in Plan
should not be public). Maybe we should check the visibility of Plan
's methods, and have another PR to tidy it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's still possible to access spaces from a plan reference?
Lines 142 to 155 in 7081e86
pub trait Plan: 'static + HasSpaces + Sync + Downcast { /// Get the plan constraints for the plan. /// This returns a non-constant value. A constant value can be found in each plan's module if needed. fn constraints(&self) -> &'static PlanConstraints; /// Create a copy config for this plan. A copying GC plan MUST override this method, /// and provide a valid config. fn create_copy_config(&'static self) -> CopyConfig<Self::VM> { // Use the empty default copy config for non copying GC. CopyConfig::default() } /// Get a immutable reference to the base plan. `BasePlan` is included by all the MMTk GC plans. fn base(&self) -> &BasePlan<Self::VM>; Lines 326 to 334 in 7081e86
pub struct BasePlan<VM: VMBinding> { pub(crate) global_state: Arc<GlobalState>, pub options: Arc<Options>, pub gc_trigger: Arc<GCTrigger<VM>>, // Spaces in base plan #[cfg(feature = "code_space")] #[space] pub code_space: ImmortalSpace<VM>,
BasePlan
is not public, though it appears in a public trait. It is the same case for Space
in Plan::collection_required
before this PR. I am not sure what is Rust's rule on this. There is a lint private_in_public
, but it seems Rust does not warn for these cases.
This PR allows bindings to create a delegated GC trigger.
GCTriggerPolicy
public. Add a new typeSpaceStats
which simply wrapsSpace
.Collection::create_gc_trigger
. This will be called when thegc_trigger
option is set toDelegated
.