CRR (S/M): 7092309: G1: introduce old region set
Tony Printezis
tony.printezis at oracle.com
Thu Nov 3 16:02:51 UTC 2011
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
>>>>>
>>>>
>
More information about the hotspot-gc-dev
mailing list