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

Allow binding to implement GC trigger #1083

Merged
merged 5 commits into from
Feb 21, 2024
Merged

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Feb 19, 2024

This PR allows bindings to create a delegated GC trigger.

  • Make GCTriggerPolicy public. Add a new type SpaceStats which simply wraps Space.
  • Add Collection::create_gc_trigger. This will be called when the gc_trigger option is set to Delegated.

@qinsoon qinsoon marked this pull request as ready for review February 20, 2024 01:13
@qinsoon qinsoon added the PR-extended-testing Run extended tests for the pull request label Feb 20, 2024
@caizixian caizixian requested review from wenyuzhao and caizixian and removed request for caizixian February 21, 2024 02:49
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

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)
Copy link
Member

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)

Copy link
Member

@wenyuzhao wenyuzhao Feb 21, 2024

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?

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>;

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>,

Copy link
Member Author

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)

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.

Copy link
Member Author

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?

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>;

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.

@qinsoon qinsoon added this pull request to the merge queue Feb 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 21, 2024
@qinsoon qinsoon added this pull request to the merge queue Feb 21, 2024
Merged via the queue into mmtk:master with commit 86a94ca Feb 21, 2024
28 of 29 checks passed
@qinsoon qinsoon deleted the vm-gc-trigger branch February 21, 2024 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-extended-testing Run extended tests for the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants