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

[John Cuthbertson]

Hi Tony,

This looks good to me. The only suggestion I have would be using a 
scoped object to handle the set_phase/clear_phase calls - but that's a nit.

JohnC

On 10/06/11 09:42, Tony Printezis wrote:
> 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
>>>>
>>>