-
Notifications
You must be signed in to change notification settings - Fork 68
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
Generational plans set next_gc_full_heap
in end_of_gc
#714
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -223,6 +223,11 @@ impl<VM: VMBinding> GCWork<VM> for EndOfGC { | |||||||||||||
fn do_work(&mut self, worker: &mut GCWorker<VM>, mmtk: &'static MMTK<VM>) { | ||||||||||||||
info!("End of GC"); | ||||||||||||||
|
||||||||||||||
// We assume this is the only running work packet that accesses plan at the point of execution | ||||||||||||||
#[allow(clippy::cast_ref_to_mut)] | ||||||||||||||
let plan_mut: &mut dyn Plan<VM = VM> = unsafe { &mut *(&*mmtk.plan as *const _ as *mut _) }; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this everywhere -- maybe we should just change the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same pattern as our prepare/release code. This is unfortunate. mmtk-core/src/scheduler/gc_work.rs Lines 44 to 46 in 2300ce1
mmtk-core/src/scheduler/gc_work.rs Lines 113 to 115 in 2300ce1
We can fix it, but not as simple as making it Basically what we want is something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it not something like an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There are many things that we need to tidy up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah right sorry -- you need a I guess we can flag this issue for the future. Could you add a comment here (and maybe the other places as well) saying that this is only safe under the assumption we're the only executing work packet. |
||||||||||||||
plan_mut.end_of_gc(worker.tls); | ||||||||||||||
|
||||||||||||||
#[cfg(feature = "extreme_assertions")] | ||||||||||||||
if crate::util::edge_logger::should_check_duplicate_edges(&*mmtk.plan) { | ||||||||||||||
// reset the logging info at the end of each 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.
Personally, I find the "release" terminology a bit confusing then. What are the semantics of "release" supposed to be exactly? Most (if not all?) plans just release pages from spaces. Is the issue that the
plan.release()
is called at the start of theRelease
work bucket instead of the end? The comment for theRelease
work packet implies that there should only ever one [1] (and only one is ever made in thescheduler_common_work
function). What is the source of the issue you have encountered? Reference processing?[1]:
mmtk-core/src/scheduler/gc_work.rs
Lines 91 to 97 in fc144cf
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.
Ah I see -- it seems like MarkSweep and Immix uses the
Release
work bucket for sweeping [1, 2]. My question is then why not have a separateSweep
work bucket like we have with theCompact
work bucket [3]?[1]: https://github.com/mmtk/mmtk-core/blob/master/src/policy/marksweepspace/malloc_ms/global.rs#L472-L501
[2]: https://github.com/mmtk/mmtk-core/blob/master/src/policy/immix/immixspace.rs#L316-L340
[3]:
mmtk-core/src/scheduler/work_bucket.rs
Line 220 in 43a6755
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.
Yeah, right. That is the motivation. The problem is that some plans generate work in the release bucket, and we need a hook to run some stuff after every work packet is done.
What you pointed out reminds me that our current code is actually incorrect. This is a race condition: we run
Release
work (which callsPlan.release()
with a mutable plan) and those sweeping work (which can accessPlan
).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.
The comment for the
Release
work packet above, seems to imply that it should not be executing with anything else and that "all the GC work" should have been finished before theRelease
work packet is called. If MS and Immix are performing sweeping inRelease
, then all the GC work hasn't really been finished right? Either we should fix the comment for theRelease
work packet, or make a new work bucket calledSweep
where MS and Immix generate sweep work packets (in the latter case, the issue of "parallel release work" ceases to exist, unless reference processing is still an issue).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.
I think that "fixing" the comment is not actually a fix. Yes -- there is a race condition. The
Sweep
work bucket is the more sensible solution (unless you have a better proposal).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.
That makes sense.
I think this is the same issue.
It doesn't look like an issue that can be solved in one or two days. To ensure we have only one thread having access to the plan, we must stop spreading
&'static Plan
(and &'static MMTk, too) everywhere, and have precise control of who holds reference toPlan
(andMMTk
as well). A key to the solution will be the ability to "revoke" all such references during STW. In fact, this is what STW is really meant to do, i.e. stop mutators from accessing the heap, as a form of synchronisation.The good thing for RefEnqueue is that it never needs to access the MMTk instance. It can be a pure VM-side operation.
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.
Yes I agree. Though with regard to this PR (or more specifically, the issue this PR is trying to address), I think the most pragmatic approach is to add a new
Sweep
orCollect
work bucket and make it so that MS and Immix generate their sweeping work packets into this bucket. In that case we don't really need this PR as all the GC work will have been done before theshould_next_gc_full_heap()
is called.It will also be helpful to exactly clarify what the semantics of the "Release" work bucket is, i.e. should we send the list of processed references to the binding in the "Release" work bucket or the "Sweep"/"Collect" work bucket, or even the "EndOfGC" work packet.
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.
It technically accesses the
mmtk
instance (right now at least -- unsure of your proposed changes):https://github.com/mmtk/mmtk-core/blob/master/src/util/reference_processor.rs#L556
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.
I have made an attempt to do this in my branch (see below), but not as precise as you described.
https://github.com/wks/mmtk-core/blob/165b3fab07cc788ad88876effdeefe567697aaf2/src/scheduler/work_bucket.rs#L258
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.
It's accessing the
mmtk.reference_processor
field. Currently it is a field of theMMTK
struct. In my branch, it is part of the VM binding instance.