RFR (M): 8153503: Move remset scan iteration claim to remset local data structure
derek.white at oracle.com
Fri Apr 8 22:34:38 UTC 2016
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
> 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.
> jprt, vm.gc multiple times, a few benchmarks
I've only started looking at the micro-level, not the whole picture. But
the new code is more easier to understand!
Here are some partial comments:
"references that point into the heap". Don't all references point
into the heap?
- Should say "into heap region", or "into collection set"?
- Is G1RemSetScanState really only used during evacuation?
- Should mention it's the scan state for the whole heap (or all heap
Lines 62, 65: Wasn't clear by the field names that these are arrays, not
single structs. Maybe make the names plural? "_iter_states", "_iter_claims"
Lines 62, 65: Should declare "_iter_states" and "_iter_claims" volatile
to go with the Atomic accessors?
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.
Line 89: Add an assert that _iter_state and _iter_claimed are not NULL.
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.
More information about the hotspot-gc-dev