Rename Workbucket's active flag to open, add enable flag by qinsoon · Pull Request #1373 · mmtk/mmtk-core

qinsoon · 2025-08-22T04:59:17Z

This PR refactors work bucket to allow disabling a bucket and to support concurrent GC.

Add a field WorkBucket::enabled: if a bucket is disabled, it behaves the same as if the bucket/stage does not exist for scheduling, except that users can add work to a disabled bucket. There are mostly two use cases for this: 1. a plan can disable buckets that the plan does not use, and 2. a concurrent plan may push concurrent work to the disabled concurrent bucket during STW, and enable the bucket at the end of a STW.
Add WorkBucketStage::Concurrent: this is unused in this PR.
Clean up most checks about stages in scheduler.rs, and move them to work_bucket.rs.
Add a reference to the scheduler in BasePlan.

wks

The algorithm should be OK because we haven't actually added concurrent GC, and I think this PR is compatible with existing plans.

One problem is that the newly introduced terms "enabled/disabled" is somewhat confusing when used together with "active/inactive" or "activated/deactivated". The doc comment of WorkBucket::deactivate even says "disable the bucket". I suggest renaming "active" to "open". Then "open/closed" means if the bucket is open during a GC as a result of, for example, draining previous buckets. Meanwhile, "enabled/disabled" means whether we hide a bucket from mutators as if the bucket doesn't exist.

wks · 2025-08-22T10:35:44Z

+        !self.is_concurrent()
+    }
+
+    /// Is this stage concurrent (which may be executedd during mutator time)?


Suggested change

/// Is this stage concurrent (which may be executedd during mutator time)?

/// Is this stage concurrent (which may be executed during mutator time)?

wks · 2025-08-22T10:48:20Z

+        self.is_disabled() || (self.is_activated() && self.is_empty())
    }

    /// Disable the bucket


This comment says "disable the bucket"

wks · 2025-08-25T01:52:34Z

+        let mut items = Vec::new();
+
+        {
+            // Drain queue by stealing until empty
+            loop {
+                match self.queue.steal() {
+                    crossbeam::deque::Steal::Success(work) => {
+                        items.push(work);
+                    }
+                    crossbeam::deque::Steal::Retry => continue,
+                    crossbeam::deque::Steal::Empty => break,
+                }
+            }
+        }
+
+        // Format collected items (just type names or Debug, depending on GCWork)
+        let debug_items: Vec<String> = items
+            .iter()
+            .map(|i| i.get_type_name().to_string()) // placeholder since GCWork isn’t Debug
+            .collect();
+
+        // Push items back into the queue
+        {
+            for work in items {
+                self.queue.push(work);
+            }
+        }
+
+        f.debug_struct("BucketQueue")
+            .field("queue", &debug_items)
+            .finish()
+    }
+}


This part drains the bucket to print the packets, and re-fill the packets. If this happens while some GC workers are fetching packets, the other worker may erroneously think the bucket is already drained.

If we really want to debug work buckets in such a detail, we should add a dedicated method, such as BucketQueue::debug_dump_packets. The Debug trait should be side-effect-free.

wks · 2025-08-25T01:54:18Z

        }
        if let Some(id) = error_example {
-            panic!("Some active buckets (such as {:?}) are not empty.", id);
+            panic!("Some open buckets (such as {:?}) are not empty.", id);


This says "open".

wks · 2025-08-25T01:55:05Z

+                continue;
+            }
+            debug!("Checking if {:?} can be opened...", id);
            let bucket_opened = bucket.update(self);


This line also says "opened".

wks · 2025-08-25T02:05:59Z


 pub struct WorkBucket<VM: VMBinding> {
+    /// Whether this bucket has been opened. Work from an open bucket can be fetched by workers.
    active: AtomicBool,


Change this to "open".

wks · 2025-08-25T02:11:52Z

+    pub const fn is_always_activated(&self) -> bool {
+        matches!(self, WorkBucketStage::Unconstrained)
+    }


Suggested change

pub const fn is_always_activated(&self) -> bool {

matches!(self, WorkBucketStage::Unconstrained)

}

pub const fn is_always_open(&self) -> bool {

matches!(self, WorkBucketStage::Unconstrained)

}

wks · 2025-08-25T02:12:06Z

+    }
+
+    /// Is this stage activated by default?
+    pub const fn is_activated_by_default(&self) -> bool {


Suggested change

pub const fn is_activated_by_default(&self) -> bool {

pub const fn is_open_by_default(&self) -> bool {

wks · 2025-08-25T02:12:36Z

+    pub const fn is_disabled_by_default(&self) -> bool {
+        matches!(self, WorkBucketStage::Concurrent)
+    }


Suggested change

pub const fn is_disabled_by_default(&self) -> bool {

matches!(self, WorkBucketStage::Concurrent)

}

pub const fn is_enabled_by_default(&self) -> bool {

!matches!(self, WorkBucketStage::Concurrent)

}

wks · 2025-08-25T02:36:12Z

+    pub(crate) fn new(stage: WorkBucketStage, active: bool, monitor: Arc<WorkerMonitor>) -> Self {
        Self {
            active: AtomicBool::new(active),


If a stage knows if the bucket at that stage is open by default, we can just initialize it here.

Suggested change

pub(crate) fn new(stage: WorkBucketStage, active: bool, monitor: Arc<WorkerMonitor>) -> Self {

Self {

active: AtomicBool::new(active),

pub(crate) fn new(stage: WorkBucketStage, monitor: Arc<WorkerMonitor>) -> Self {

Self {

active: AtomicBool::new(stage.is_open_by_default()),

qinsoon · 2025-08-25T04:35:20Z

@wks I have made changes as suggested. Can you review again?

wks

There are still other places in the comments that mention "activated buckets". We should grep the word "activate" and change it to "open" or "opened", depending on the context.

We should update the pull request description #1373 (comment), too, as it will become the commit log.

qinsoon · 2025-08-25T05:27:32Z

There are still other places in the comments that mention "activated buckets". We should grep the word "activate" and change it to "open" or "opened", depending on the context.

We should update the pull request description #1373 (comment), too, as it will become the commit log.

Done.

wks

LGTM.

I also pushed some pedantic changes and synchronized with master. You can merge this PR when you think it is appropriate.

Refactoring work bucket to support concurrent GC

c1489fe

qinsoon force-pushed the bucket-disable-refactoring branch from e49253d to c1489fe Compare August 22, 2025 05:01

qinsoon added the PR-extended-testing Run extended tests for the pull request label Aug 22, 2025

Missing doc

b504407

qinsoon marked this pull request as ready for review August 22, 2025 06:01

qinsoon requested a review from wks August 22, 2025 06:01

wks requested changes Aug 25, 2025

View reviewed changes

Renaming active to open and other minor changes based on the review.

0f64b73

qinsoon force-pushed the bucket-disable-refactoring branch from 1a3e5ea to 0f64b73 Compare August 25, 2025 04:09

Remove dumping priority queue

0548986

wks reviewed Aug 25, 2025

View reviewed changes

Comment thread src/scheduler/work_bucket.rs Outdated

Comment thread src/scheduler/work_bucket.rs Outdated

Rename activate to open in comments

967dfa7

qinsoon changed the title ~~Refactoring to support disabling work bucket and concurrent GC~~ Rename Workbucket's active flag to open, add enable flag Aug 25, 2025

wks added 2 commits August 25, 2025 14:03

Change field order in constructor

95df968

Merge branch 'master' into review/bucket-disable-refactoring

917d123

wks approved these changes Aug 25, 2025

View reviewed changes

qinsoon added this pull request to the merge queue Aug 25, 2025

Merged via the queue into mmtk:master with commit 6eb1328 Aug 25, 2025
31 of 33 checks passed

qinsoon deleted the bucket-disable-refactoring branch August 25, 2025 10:14

	/// Is this stage concurrent (which may be executedd during mutator time)?
	/// Is this stage concurrent (which may be executed during mutator time)?

	pub const fn is_activated_by_default(&self) -> bool {
	pub const fn is_open_by_default(&self) -> bool {

Conversation

qinsoon commented Aug 22, 2025 • edited by wks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinsoon commented Aug 25, 2025

Uh oh!

wks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

qinsoon commented Aug 25, 2025

Uh oh!

wks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qinsoon commented Aug 22, 2025 •

edited by wks

Loading

wks left a comment •

edited

Loading