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

[Bengt Rutisson]

John,

Thanks for removing the asserts! 

Hope your push succeeds this time. You do know about the "-extratime 5m" workaround for the failure that you got, right?

Bengt

15 dec 2011 kl. 18:55 skrev John Cuthbertson <john.cuthbertson at oracle.com>:

> 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/7d595faf/attachment.htm>