From feb5471bb34d31d9386f7182cc67f99f2c30944a Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Wed, 25 Oct 2023 17:45:30 +1300 Subject: [PATCH 1/3] Use `cargo generate-lockfile` to update JikesRVM's Cargo.lock (#996) `auto-merge.yml` uses `cargo build` to generate lockfile for most bindings. But this does not work for JikesRVM: 1. JikesRVM uses `--target i686-unknown-linux-gnu` which is not installed automatically when running `cargo build`. 2. JikesRVM's cargo project uses some Rust source files that are generated during building JikesRVM. So we cannot build the cargo project alone from a fresh repo clone. We have to do a full build. As a workaround, we use `cargo generate-lockfile` for JikesRVM. `cargo generate-lockfile` will update the version of dependencies. But we do not have a better option for JikesRVM --- .github/workflows/auto-merge.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/auto-merge.yml b/.github/workflows/auto-merge.yml index 7ca6dd2320..9bb5b7af9c 100644 --- a/.github/workflows/auto-merge.yml +++ b/.github/workflows/auto-merge.yml @@ -50,7 +50,11 @@ jobs: base_repo: mmtk/mmtk-jikesrvm ref: ${{ needs.binding-refs.outputs.jikesrvm_binding_ref }} core_commit: ${{ needs.get-merged-pr.outputs.commit }} - update_lockfile: cargo build --features nogc --target i686-unknown-linux-gnu + # `cargo generate-lockfile` will update other dependencies. We avoid using it for the bindings. + # But we do not have a good option for JikesRVM. The Rust project in JikesRVM needs some source files + # that are generated during its build process. Unless we want to do a full build for JikesRVM, we cannot + # use `cargo build`. So use `cargo generate-lockfile` instead. + update_lockfile: cargo generate-lockfile secrets: inherit check-merge-v8-pr: From abd38ec31064e7d52ae2b492a75b4bcc7f7cc554 Mon Sep 17 00:00:00 2001 From: Yi Lin Date: Wed, 25 Oct 2023 18:05:09 +1300 Subject: [PATCH 2/3] Use BumpPointer::default() (#993) This PR changes the code snippet in our doc to use `BumpPointer::default()` to create a new zero-initialized bump pointer struct. It also changes our internal implementation to use `BumpPointer::default()` instead of `BumpPointer::new(zero, zero)`. --- src/util/alloc/bumpallocator.rs | 18 +++++++----------- src/util/alloc/immix_allocator.rs | 4 ++-- .../dummyvm/src/tests/doc_mutator_storage.rs | 18 ++++-------------- 3 files changed, 13 insertions(+), 27 deletions(-) diff --git a/src/util/alloc/bumpallocator.rs b/src/util/alloc/bumpallocator.rs index 8b993ce5e3..98c33fc56d 100644 --- a/src/util/alloc/bumpallocator.rs +++ b/src/util/alloc/bumpallocator.rs @@ -26,6 +26,9 @@ pub struct BumpAllocator { /// A common fast-path bump-pointer allocator shared across different allocator implementations /// that use bump-pointer allocation. +/// A `BumpPointer` is always initialized with cursor = 0, limit = 0, so the first allocation +/// always fails the check of `cursor + size < limit` and goes to the slowpath. A binding +/// can also take advantage of this design to zero-initialize the a bump pointer. #[repr(C)] #[derive(Copy, Clone)] pub struct BumpPointer { @@ -34,13 +37,6 @@ pub struct BumpPointer { } impl BumpPointer { - pub const fn new(start: Address, end: Address) -> Self { - BumpPointer { - cursor: start, - limit: end, - } - } - pub fn reset(&mut self, start: Address, end: Address) { self.cursor = start; self.limit = end; @@ -48,10 +44,10 @@ impl BumpPointer { } impl std::default::Default for BumpPointer { + /// Defaults to 0,0. In this case, the first + /// allocation would naturally fail the check + /// `cursor + size < limit`, and go to the slowpath. fn default() -> Self { - // Defaults to 0,0. In this case, the first - // allocation would naturally fail the check - // `cursor + size < limit`, and go to the slowpath. BumpPointer { cursor: Address::ZERO, limit: Address::ZERO, @@ -180,7 +176,7 @@ impl BumpAllocator { ) -> Self { BumpAllocator { tls, - bump_pointer: unsafe { BumpPointer::new(Address::zero(), Address::zero()) }, + bump_pointer: BumpPointer::default(), space, context, } diff --git a/src/util/alloc/immix_allocator.rs b/src/util/alloc/immix_allocator.rs index 994b5929ba..077aaefbd9 100644 --- a/src/util/alloc/immix_allocator.rs +++ b/src/util/alloc/immix_allocator.rs @@ -174,10 +174,10 @@ impl ImmixAllocator { tls, space: space.unwrap().downcast_ref::>().unwrap(), context, - bump_pointer: BumpPointer::new(Address::ZERO, Address::ZERO), + bump_pointer: BumpPointer::default(), hot: false, copy, - large_bump_pointer: BumpPointer::new(Address::ZERO, Address::ZERO), + large_bump_pointer: BumpPointer::default(), request_for_large: false, line: None, } diff --git a/vmbindings/dummyvm/src/tests/doc_mutator_storage.rs b/vmbindings/dummyvm/src/tests/doc_mutator_storage.rs index b48dcbdea6..48734626bf 100644 --- a/vmbindings/dummyvm/src/tests/doc_mutator_storage.rs +++ b/vmbindings/dummyvm/src/tests/doc_mutator_storage.rs @@ -76,19 +76,9 @@ pub fn embed_fastpath_struct() { // Bind an MMTk mutator let mutator = mmtk::memory_manager::bind_mutator(&fixture.mmtk, tls_opaque_pointer); - // Create a fastpath BumpPointer - let default_bump_pointer = { - // First find the allocator - let selector = mmtk::memory_manager::get_allocator_mapping( - &fixture.mmtk, - AllocationSemantics::Default, - ); - let default_allocator = unsafe { - mutator.allocator_impl::>(selector) - }; - // Copy the bump pointer struct - default_allocator.bump_pointer - }; + // Create a fastpath BumpPointer with default(). The BumpPointer from default() will guarantee to fail on the first allocation + // so the allocation goes to the slowpath and we will get an allocation buffer from MMTk. + let default_bump_pointer = BumpPointer::default(); // Store the fastpath BumpPointer along with the mutator let mut storage = MutatorInTLS { default_bump_pointer, @@ -119,7 +109,7 @@ pub fn embed_fastpath_struct() { default_allocator.bump_pointer = storage.default_bump_pointer; // Do slow path allocation with MMTk let addr = default_allocator.alloc_slow(size, 8, 0); - // Copy bump pointer values to the fastpath BumpPointer + // Copy bump pointer values to the fastpath BumpPointer so we will have an allocation buffer. storage.default_bump_pointer = default_allocator.bump_pointer; addr } From 59ff055412109eeab0169b314cc18a1d68274fb0 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Wed, 25 Oct 2023 21:59:37 +0800 Subject: [PATCH 3/3] Update doc comments (#1000) We removed the feature for letting the coordinator call both `stop_all_mutators` and `resume_mutators` since https://github.com/mmtk/mmtk-core/pull/794 and https://github.com/mmtk/mmtk-core/pull/814. The documentation should reflect that. --- src/vm/collection.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/vm/collection.rs b/src/vm/collection.rs index 7c916eb302..9204d6b0a8 100644 --- a/src/vm/collection.rs +++ b/src/vm/collection.rs @@ -12,14 +12,13 @@ pub enum GCThreadContext { /// VM-specific methods for garbage collection. pub trait Collection { /// Stop all the mutator threads. MMTk calls this method when it requires all the mutator to yield for a GC. - /// This method is called by a single thread in MMTk (the GC controller). /// This method should not return until all the threads are yielded. /// The actual thread synchronization mechanism is up to the VM, and MMTk does not make assumptions on that. /// MMTk provides a callback function and expects the binding to use the callback for each mutator when it /// is ready for stack scanning. Usually a stack can be scanned as soon as the thread stops in the yieldpoint. /// /// Arguments: - /// * `tls`: The thread pointer for the GC controller/coordinator. + /// * `tls`: The thread pointer for the GC worker. /// * `mutator_visitor`: A callback. Call it with a mutator as argument to notify MMTk that the mutator is ready to be scanned. fn stop_all_mutators(tls: VMWorkerThread, mutator_visitor: F) where @@ -27,8 +26,11 @@ pub trait Collection { /// Resume all the mutator threads, the opposite of the above. When a GC is finished, MMTk calls this method. /// + /// This method may not be called by the same GC thread that called `stop_all_mutators`. + /// /// Arguments: - /// * `tls`: The thread pointer for the GC controller/coordinator. + /// * `tls`: The thread pointer for the GC worker. Currently it is the tls of the embedded `GCWorker` instance + /// of the coordinator thread, but it is subject to change, and should not be depended on. fn resume_mutators(tls: VMWorkerThread); /// Block the current thread for GC. This is called when an allocation request cannot be fulfilled and a GC