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

[John Cuthbertson]

Hi Tony,

Looks good!.

JohnC

On 11/03/11 09:02, Tony Printezis wrote:
> John,
>
> Better?
>
> http://cr.openjdk.java.net/~tonyp/7092309/webrev.2/
>
> Tony
>
> On 10/31/2011 07:40 PM, John Cuthbertson wrote:
>> 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
>>>>>>
>>>>>
>>