Skip to content

Allow MockVM to run GC - #1408

Draft
qinsoon wants to merge 11 commits into
mmtk:masterfrom
qinsoon:mock-vm-gc
Draft

qinsoon wants to merge 11 commits into
mmtk:masterfrom
qinsoon:mock-vm-gc

Conversation

@qinsoon

@qinsoon qinsoon commented Oct 24, 2025 •

Copy link
Copy Markdown
Member

This PR made some changes to MockVM to allow us to run GC in the tests.

  • Add features mock_test_side_metadata and mock_test_header_metadata. We only test with mock_test_side_metadata (as some plans don't run with header metadata).
  • No longer use lock for the MockVM instance (otherwise it will cause deadlock when we reentry MockVM)
  • Statically initialize a MMTK instance for mock tests.
  • Mock tests now use mock_api to create a mutator. This registers the mutator to mock VM, and can be blocked for GC.
  • Mock tests now properly initialize GC threads, and handle panics in GC threads.
  • Minor changes to print more information for mmap failures.

@qinsoon qinsoon added the PR-extended-testing Run extended tests for the pull request label Oct 27, 2025
@qinsoon
qinsoon requested a review from wks October 27, 2025 02:45
@qinsoon
qinsoon marked this pull request as ready for review October 27, 2025 02:46
@wks

wks commented Oct 27, 2025 •

Copy link
Copy Markdown
Collaborator

std::sync::ReentrantLock is an unstable feature. But a third-party crate parking_lot has an implementation of ReentrantMutex. If that is helpful, we can add the parking_lot as a dependency when the mock_test feature is enabled.

}*/

pub fn reset_cursor(&self, top: Address) {
// top might be Address::ZERO

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is where I think Option<NonZeroAddress> can be useful. See #1223

Comment thread src/util/linear_scan.rs Outdated
fn next(&mut self) -> Option<<Self as Iterator>::Item> {
while self.cursor < self.end {
if !self.cursor.is_aligned_to(ObjectReference::ALIGNMENT) {
self.cursor += VM::MIN_ALIGNMENT;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Currently ObjectReference is required to be aligned to ObjectReference::ALIGNMENT. So even if VM::MIN_ALIGNMENT is smaller than ObjectReference::ALIGNMENT, VO bits can only be set at addresses which are multiples of ObjectReference::ALIGNMENT. I think the algorithm here is wrong (or just outdated). It is pointless to increment the cursor by VM::MIN_ALIGNMENT. It should simply align the cursor up to the next multiple of ObjectReference::ALIGNMENT. And I wonder whether find_last_non_zero_bit_in_metadata_bytes could be more efficient (needs to search forward instead of backward).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is resolved in 040cfc9.

Conflict resolution and fixes needed after the merge:
- Adopt master's disable_collection()/enable_collection() API in place of the
  removed MockVM::is_collection_enabled.
- Add GLOBAL_FIELD_UNLOG_BIT_SPEC to both mock metadata layouts, and keep the
  mark bit on the side in the header layout (required by LXR).
- Make header metadata the default when mock_test_side_metadata is not set, so
  `--features mock_test` alone builds (used by ci-build.sh), and reject
  enabling both layouts.
- Drop the mock-only side metadata base address; master computes it dynamically.
- Run LXR mock tests with header metadata in ci-test.sh, as LXR requires
  in-header forwarding bits.
- Port new master mock tests to this branch's MockVM: use fixture.mutator(),
  pass `force` to handle_user_collection_request, wrap mock_test_shutdown in
  with_mockvm, and stop the OOM-unwind test from spawning GC workers.
- Use inspect_err now that MSRV is 1.84.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@qinsoon qinsoon removed the PR-extended-testing Run extended tests for the pull request label Sep 23, 2026
@qinsoon qinsoon mentioned this pull request Sep 23, 2026
@qinsoon
qinsoon marked this pull request as draft September 24, 2026 05:44
@qinsoon

qinsoon commented Sep 24, 2026

Copy link
Copy Markdown
Member Author

I turned this PR back to a draft for now. I will wait for #1601 (aarch64 linux) and #1599 (aarch64 macOS) to merge first, then update this PR to test on those platforms.

Resolve conflicts in the two allocate_no_gc_oom_on_acquire mock tests: keep
this branch's fixture.mutator() API, and take master's page-sized allocation
request.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants