Skip to content

Update to MMTk core PR #817 #218

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

Closed
wants to merge 3 commits into from
Closed

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented May 17, 2023

Update for mmtk/mmtk-core#817.

@qinsoon qinsoon marked this pull request as ready for review May 17, 2023 05:13
Copy link
Contributor

@wks wks left a comment

Choose a reason for hiding this comment

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

Obviously this PR is a quick hack to adapt to the new API. Although we can make it more idiomatic in a subsequent PR, I prefer we do it now, because implementing the API in an idiomatic way can help us check if this API is properly designed. If we find it difficult to implement the OpenJDKMutatorIterator by wrapping a JavaThreadIteratorWithHandle (for example, due to lifetime issues), we may need to adjust the signature of the ActivePlan::mutators() method.

use std::sync::{Mutex, MutexGuard};

struct OpenJDKMutatorIterator<'a> {
_guard: MutexGuard<'a, ()>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Guarding the global iterator instance (static MaybeUninit<JavaThreadIteratorWithHandle> jtiwh;) in C++ is one way to adapt to the new API. It is as correct as the previous code.

But given that OpenJDK provides a proper iterator type JavaThreadIteratorWithHandle, I think a more straightforward way to implement OpenJDKMutatorIterator is wrapping an instance of JavaThreadIteratorWithHandle (or a smart pointer to it) right into the OpenJDKMutatorIterator itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I removed the global iterator, and moved that to each iterator. JavaThreadIteratorWithHandle is a StackObj, thus cannot be allocated in the heap. I create an uninitialized instance in Rust, and pass that to C++ to let it initialize the instance.

@qinsoon qinsoon force-pushed the update-mmtk-core-817 branch from 3458f47 to bbb6e0d Compare May 19, 2023 02:04
Copy link
Contributor

@wks wks left a comment

Choose a reason for hiding this comment

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

The obvious problem is that the Rust code now knows the layout of the C++ struct JavaThreadIteratorWithHandle. This makes the code much less maintainable, especially if the layout changes in unspecified ways at different debug levels (and the Rust code must match the C++-side debug level).

From the code, I found that the Thread class has pointers back to some internal structures of JavaThreadIteratorWithHandle (see comments). This prevents us from instantiating JavaThreadIteratorWithHandle on the stack and then copying it to the Rust Iterator. I also tried working around this by using the placement new syntax to initialise JavaThreadIteratorWithHandle in a pre-allocated buffer, but it failed too. It looks like StackObj also prevented placement new by defining private new methods.

Perhaps the safest way is still switching to internal iteration.

static MaybeUninit<JavaThreadIteratorWithHandle> jtiwh;
static bool mutator_iteration_start = true;
static void mmtk_new_java_thread_iterator(void* iter) {
*(JavaThreadIteratorWithHandle*)iter = JavaThreadIteratorWithHandle();
Copy link
Contributor

Choose a reason for hiding this comment

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

This step moves the constructed JavaThreadIteratorWithHandle instance to another location.

However, part of the race-prevention algorithm in OpenJDK includes letting each Thread hold a pointer to the SafeThreadsListPtr instance inside JavaThreadIteratorWithHandle. If the SafeThreadsListPtr is moved, the field in Thread becomes a dangling pointer.

See

Comment on lines +96 to +98
pub new_java_thread_iterator: extern "C" fn(iter: *mut abi::JavaThreadIteratorWithHandle),
pub java_thread_iterator_next:
extern "C" fn(iter: *mut abi::JavaThreadIteratorWithHandle) -> *mut Mutator<OpenJDK>,
Copy link
Contributor

Choose a reason for hiding this comment

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

There is one missing upcall: the destructor. JavaThreadIteratorWithHandle has a destructor which should be called when the iterator is dropped.

@wks
Copy link
Contributor

wks commented May 22, 2023

Maybe the easiest solution is just collecting the mutator pointers in a Vec and embed the Vec into the Iterator<Mutator>.

The following is the typical use case in mmtk-core:

        for mutator in <C::VM as VMBinding>::VMActivePlan::mutators() {
            mmtk.scheduler.work_buckets[WorkBucketStage::Prepare]
                .add(PrepareMutator::<C::VM>::new(mutator));
        }

It creates one work packet for each mutator. This means we assume the mutator pointer to be valid during the GC anyway, even after the iterator returned by mutators() returned. Under such assumption, it should be safe to just collect the mutator pointers.

@qinsoon
Copy link
Member Author

qinsoon commented May 22, 2023

Closed, in favor of #220.

@qinsoon qinsoon closed this May 22, 2023
qinsoon pushed a commit that referenced this pull request May 26, 2023
This is an alternative approach for #218 that addresses the upstream API change in mmtk/mmtk-core#817.  This PR simply saves the mutator pointers into a `VecDeque` and embeds it into the iterator.
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