RFR(S): 7119908: G1: Cache CSet start region for each worker for subsequent reuse

[Bengt Rutisson]

John,

Looks good to me.

One minor thing. I think the two asserts on lines 2710 and 2711 are 
unnecessary. They will never fail.

2709 HeapRegion* G1CollectedHeap::start_cset_region_for_worker(int 
worker_i) {
2710   assert(_worker_cset_start_region != NULL, "sanity");
2711   assert(_worker_cset_start_region_time_stamp != NULL, "sanity");

In the constructor for G1CollectedHeap you set the arrays up using 
NEW_C_HEAP_ARRAY. If that fails to allocate memory it will call 
vm_exit_out_of_memory(). So, you should never get to 
start_cset_region_for_worker() with the arrays being NULL.

Bengt


On 2011-12-14 23:23, John Cuthbertson wrote:
> Hi Everyone,
>
> Based upon some further comments from Tony, I have another new webrev 
> for this change. It can be found at: 
> http://cr.openjdk.java.net/~johnc/7119908/webrev.2/
>
> Changes in this version include:
>
> * some minor name changes
> * the code that calculates the starting region was using 
> total_workers() when it should have been using active_workers()
> * minor tweaks to the start and end index values to get a slightly 
> better distribution.
>
> Testing:
> GC test suite
>
> Thanks,
>
> JohnC
>
> On 12/13/11 10:39, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> I have a new webrev based upon comments from Tony, which can be found 
>> at: http://cr.openjdk.java.net/~johnc/7119908/webrev.1/
>>
>> Changes in this version include:
>>
>> * Using the value of the existing G1CollectedHeap::_gc_time_stamp 
>> field, rather than the value of _total_collections, as the TS values.
>> * A small optimization that may reduce the number of iterations while 
>> walking the collection set to calculate the starting region for a 
>> worker. We check the entry for the previous worker and if that is 
>> valid then we starting iterating over the collection set from that 
>> region.
>>
>> Testing:
>> GC test suite; jprt.
>>
>> Thanks,
>>
>> JohnC
>>
>> On 12/09/11 11:18, John Cuthbertson wrote:
>>> Hi Everyone,
>>>
>>> Can I have a couple of volunteers to review the changes for this CR? 
>>> The webrev can be found at: 
>>> http://cr.openjdk.java.net/~johnc/7119908/webrev.0/
>>>
>>> Summary:
>>> As part of the code review comments for 7112743 (G1: Reduce overhead 
>>> of marking closure during evacuation pauses) it was suggested that 
>>> instead of recalculating the starting heap region for each worker 
>>> thread, we reuse the values calculated during RSet scanning. These 
>>> changes address that review comment. In these changes I maintain a 
>>> cache that is used and updated by 
>>> G1CollectedHeap::start_cset_region_for_worker(). The first time this 
>>> routine is called by a worker thread during an evacuation pause 
>>> (currently during RSet scanning) the cached value for the worker 
>>> gets set; when the routine is called subsequently the region that 
>>> was cached for the worker is returned. I employ a simple stamp 
>>> mechanism based upon the number of GCs that ensures the validity of 
>>> the regions in the cache and makes clearing the cache unnecessary.
>>>
>>> Testing:
>>> GC Test suite with a small marking threshold (10%) with and without 
>>> verification; the configuration of SPECjbb used to test 7117423; 
>>> Kitchensink.
>>>
>>> Thanks,
>>>
>>> JohnC
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20111215/67d822dd/attachment.htm>