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

[John Cuthbertson]

Hi Bengt,

I was just being a little bit paranoid. Since the jprt job of the push 
failed - I'll remove them.

Thanks,

JohnC

On 12/15/11 00:13, Bengt Rutisson wrote:
>
> 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/b9ae68e6/attachment.htm>