CRR (S/M): 7092309: G1: introduce old region set

[Tony Printezis]

Latest webrev based on Bengt's suggestions (I'll need to test the 
changes to make sure I didn't mess up the checks, but I'll do that in a 
few days after I've finished testing another changeset):

http://cr.openjdk.java.net/~tonyp/7092309/webrev.1/

The only differences wrt the first webrev are the changes in the 
check_mt_safety() methods in heapRegionSets.cpp.

Tony

Tony Printezis wrote:
> 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
>>>
>>