-
Notifications
You must be signed in to change notification settings - Fork 69
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
Remove GC in harness_begin. full_heap_system_gc defaults to true #1141
base: master
Are you sure you want to change the base?
Conversation
@wks Feel free to make code changes in the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that some VMs, such as CRuby, allows the user to choose between triggering a nursery or full-heap GC, we should retain a public API to allow triggering nursery and full-heap GC, too.
* The user applications and benchmarks are responsible to trigger a GC before calling `harness_begin`. | ||
* `handle_user_collection_request` | ||
* The runtime option `full_heap_sytem_gc` defaults to `true` now (it used to be `false` by default). | ||
The GC triggered by this function will be a full heap GC by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add link to this PR in a "See also:" section.
@@ -75,6 +75,23 @@ See also: | |||
- PR: <https://github.com/mmtk/mmtk-core/pull/1134> | |||
- Example: <https://github.com/mmtk/mmtk-openjdk/pull/274> | |||
|
|||
### Remove GC in `harness_begin` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the versions are in the reversed order, we should list changes in the reversed order, too.
src/mmtk.rs
Outdated
force: bool, | ||
exhaustive: bool, | ||
) { | ||
pub fn handle_user_collection_request(&self, tls: VMMutatorThread) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some VMs gives the user fine-grained control of whether to trigger a full-heap GC or a minor GC. In CRuby, the GC::start
method uses the full_mark
parameter for that. The current master branch of mmtk-ruby
ignores this because it still doesn't support StickyImmix, but will enable StickyImmix once we fix the side forwarding bits bug. We should leave a public API for the VM to choose whether to perform an exhaustive GC.
Note: memory_manager::handle_user_collection_request
is public, and MMTK::handle_user_collection_request
is a public method, too.
The OpenJDK binding calls memory_manager::handle_user_collection_request
which does not have the force
and exhaustive
parameters. I suggest we leave Option::full_heap_system_gc
and MMTK::handle_user_collection_request
unchanged, but change memory_manager::handle_user_collection_request
to pass true
and true
to MMTK::handle_user_collection_request
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change memory_manager::handle_user_collection_request to pass true and true to MMTK::handle_user_collection_request
In this case, MMTk basically ignores those two options: ignore_system_gc
is ignored, as we always force GC, and full_heap_system_gc
is also ignored as we always do full heap/exhaustive GC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. MMTK::handle_user_collection_request
is public, too. The Ruby VM, for example, can directly call mmtk.handle_user_collection_request(tls, false, true)
to trigger full-heap GC, and call mmtk.handle_user_collection_request(tls, false, false)
to trigger nursery GC. OpenJDK can call memory_manager::handle_user_collection_request(tls)
to use the default force
and exhaustive
arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I think a more elegant solution is to change the argument of MMTK::handle_user_collection_request
to take one single UserCollectionArgs
value:
struct UserCollectionArgs {
force: bool,
exhaustive: bool,
}
impl default for UserCollectionArgs {
fn default() -> Self {
Self { force: true, exhaustive: true }
}
}
This will allow the VM binding to always call mmtk.handle_user_collection_request(tls, UserCollectionArgs::default())
if they want the default value, or call mmtk.handle_user_collection_request(tls, UserCollectionArgs { exhaustive: true, ..Default::default() }
if they want to customise. But that'll be an API-breaking change.
ful_heap_system_gc from options.
As we discussed earlier, I remove the option @wks Can you take a look at this PR? If the changes look fine, I will fix the bindings for the breaking change. |
/// This is usually called by the benchmark harness as its last step before the actual benchmark. | ||
pub fn harness_begin(&self, tls: VMMutatorThread) { | ||
pub fn harness_begin(&self, _tls: VMMutatorThread) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we decided that this is going to be an API-breaking change, we can remove this unused _tls
argument.
pub fn harness_begin(&self, _tls: VMMutatorThread) { | |
pub fn harness_begin(&self) { |
@@ -403,35 +401,26 @@ impl<VM: VMBinding> MMTK<VM> { | |||
} | |||
|
|||
/// The application code has requested a collection. This is just a GC hint, and | |||
/// we may ignore it. | |||
/// we may ignore it (See `ignore_system_gc` and `full_heap_system_gc` in [`crate::util::options::Options`]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We removed full_heap_system_gc
.
/// we may ignore it (See `ignore_system_gc` and `full_heap_system_gc` in [`crate::util::options::Options`]). | |
/// we may ignore it (See `ignore_system_gc` in [`crate::util::options::Options`]). |
This PR fixes #1140.