CRR (S/M): 7092309: G1: introduce old region set
Tony Printezis
tony.printezis at oracle.com
Thu Oct 6 14:38:06 UTC 2011
Bengt,
Thanks for looking at this! See inline.
On 10/6/2011 7:04 AM, Bengt Rutisson wrote:
>
> Hi Tony,
>
> This looks good. It will be great to have the old region set for the
> CR regarding humungous allocation and concurrent mark start that I am
> working on.
Yes, this would be a nice benefit of this change and make the cycle
initiation code a bit more straightforward.
> Some comments regarding MasterOldRegionSet::check_mt_safety():
>
> The code would be easier (at least for me) to read like this:
>
> if (SafepointSynchronize::is_at_safepoint()) {
> guarantee(Thread::current()->is_VM_thread() ||
> (_phase == HRSPhaseEvacuation &&
> FreeList_lock->owned_by_self()) ||
> (_phase == HRSPhaseCleanup &&
> OldSets_lock->owned_by_self()), "Master old set MT safety protocol:
> safepoint invariants violated");
> } else {
> guarantee(Heap_lock->owned_by_self(), "Master old set MT safety
> protocol: Heap lock must be held if we are not at a safepoint");
> }
>
> It would also align nicely with how you structured the comment just
> above the guarantee.
This is a good suggestion. I'll make the change and also update the
other check_mt_safety() methods in that file this way too.
> Also, the only call to check_mt_safety() that I could find is in
> hrs_assert_mt_safety_ok() and there we call it from an assert. If that
> is correct, why are then the check_mt_safety() implementations using
> guarantees and not asserts?
The idiom:
assert(check_something(), "...");
with check_something() doing various checks (guarantees, explicitly
tests, etc.) and returns true is common in HotSpot as a way to do those
checks only in a non-product build (instead of creating a non-product
method which is a nop in the product). Having them as guarantees also
allows us to turn the assert in hrs_assert_mt_safety_ok() into a
guarantee and enable those checks in a product build, if we're looking
for a race that it's only reproduced with product builds.
Tony
> Thanks,
> Bengt
>
> On 2011-10-04 21:54, Tony Printezis wrote:
>> Hi all,
>>
>> This change adds to G1 the facility to keep track of the allocated
>> old regions with the HeapRegionSet abstraction and moves us one step
>> closer to having all regions being tracked by the same abstraction
>> (after this, only the survivor / eden regions remain). I decided to
>> work on the old region set now as it was a relatively easy change to
>> implement and will make some follow-on CRs easier to implement. The
>> webrev is here:
>>
>> http://cr.openjdk.java.net/~tonyp/7092309/webrev.0/
>>
>> Most of the changes were straightforward (I largely followed how we
>> maintain the humongous region set). The only part which I had to
>> slightly revamp was the code that tears down / rebuilds the region
>> sets. This small refactoring, though, was worthwhile as it makes that
>> code easier to extend in the future (when we introduce the eden /
>> survivor lists).
>>
>> Tony
>>
>
More information about the hotspot-gc-dev
mailing list