RFR (M): 8153503: Move remset scan iteration claim to remset local data structure
thomas.schatzl at oracle.com
Tue Apr 12 09:41:07 UTC 2016
On Fri, 2016-04-08 at 18:34 -0400, Derek White wrote:
> Hi Thomas,
> On 4/5/16 5:46 AM, Thomas Schatzl wrote:
> > Hi all,
> > can I have reviews for this refactoring of the remset scan
> > iteration
> > claim procedure?
> > First, this changeset tries to capture all remset scanning specific
> > data that is located everywhere, and only used during remembered
> > set
> > scan, into a single new G1RemsetScanState class.
> > This cleans up lots of code like members in various classes and
> > additional maintenance code all over the place (and in fact this
> > change
> > deletes more code than it adds).
> > Second, I took the liberty to change the weird scan algorithm that
> > does
> > two-passes over the collection set into a single-pass one.
> > Instead of, by setting some internal flag whether we are in the
> > second
> > pass and not do code root scan (iirc), use the result of an atomic
> > operation to make sure that only a single thread does the code root
> > scan.
> > Then the change removes the nasty _cset_rs_update_cl member of
> > G1RemSet, which hid a nasty bug (JDK-8153360) which this change
> > does
> > not address (but make obvious).
> > Also did the usual touch-up of method names that are affected.
> > Webrev:
> > https://bugs.openjdk.java.net/browse/JDK-8153503
> > CR:
> > http://cr.openjdk.java.net/~tschatzl/8153503/webrev/
> > Testing:
> > jprt, vm.gc multiple times, a few benchmarks
> > Thanks,
> > Thomas
> I've only started looking at the micro-level, not the whole picture.
> But the new code is more easier to understand!
That's the main point.
> Here are some partial comments:
> --- src/share/vm/gc/g1/g1RemSet.hpp
> Line 132:
> "references that point into the heap". Don't all references point
> into the heap?
> - Should say "into heap region", or "into collection set"?
"Into the heap" is indeed kind of redundant.
> --- src/share/vm/gc/g1/g1RemSet.cpp
> Line 49:
> - Is G1RemSetScanState really only used during evacuation?
Yes. There is no state preserved across collections. You could recreate
it from scratch at every collection. I wanted to change runtime
behavior as little as possibly by recreating it from scratch every
time, as the original call also preserved the per-region members now
collected into arrays.
> - Should mention it's the scan state for the whole heap (or all heap
I will try to clarify the comment.
> Lines 62, 65: Wasn't clear by the field names that these are arrays,
> not single structs. Maybe make the names plural? "_iter_states",
Okay. No preference.
> Lines 62, 65: Should declare "_iter_states" and "_iter_claims"
> volatile to go with the Atomic accessors?
Can do. Since all accesses are before/after Atomic cmpxchg which do the
necessary (compiler) barriers anyway, this seemed redundant.
> Line 83: Save "max_regions" as a field here, and remove the
> max_regions argument to reset(). We could then add bounds
> checks when/where needed.
> Line 86: Should G1RemSetScanState::initialize() call
> Perhaps the arrays are not used until after a call to reset(), but
> it's a little weird that "initialize()"
> doesn't actually initialize all of the memory.
It's kind of redundant as you note, as reset() is called at the start
of every collection anyway. Initialize is called during startup, I
tried to avoid this useless overhead.
> Line 89: Add an assert that _iter_state and _iter_claimed are not
> i.e. check that initialize() was called before reset().
> Line 93: Add a CR after. And lines 102, 111, 115, 119.
> Line 113: Can declare const also.
> I'll look at this more on Monday.
> - Derek
More information about the hotspot-gc-dev