RFR(S/M): 7112743: G1: Reduce overhead of marking closure during evacuation pauses

[Tony Printezis]

Bengt,

FYI: doHeapRegion() is supposed to return false if it wants the 
iteration to carry on, true if it wants the iteration to exit 
immediately (IMHO, it should have been the other way around, but anyway...).

Tony

On 11/23/2011 08:15 AM, Bengt Rutisson wrote:
>
> Hi John,
>
> Overall this looks good.
>
> Some comments:
>
> concurrentMark.cpp
>
> line 2880. return statement has wrong indentation now. A space was 
> removed.
>
> bool doHeapRegion(HeapRegion* hr)
>
> why use the completed varaible? why not just:
> while (!_bm->iterate(&_bit_cl, mr));
>
> I don't really like that the completed name is so close to 
> HeapRegionClosure::_complete
>
> I am also a bit confused about the bool return value. It seems that 
> many places in the code requires that doHeapRegion() always returns 
> false. I hope this gets cleaned up by Tony's iterator changes. I have 
> not yet had time to look at them.
>
>
>
> g1CollectedHeap.cpp
>
> G1CollectedHeap::calculateStartRegion()
>
> (I saw that you just moved the code from g1RemSet, but I still have 
> some comments regarding it.)
>
> Shouldn't the check "if (ParallelGCThreads > 0)" be "if 
> (use_parallel_gc_threads())"?
> I realize that it is the same thing, but since we have a method for it 
> I assume that we should use it. In fact, it might be more correct to 
> have the test be "if (workers()->total_workers() > 0)" since we would 
> get a division by zero if that is not true. (An alternative would be 
> to check "if (worker_i > 0)". That would be more efficient, but it 
> would rely on that single threaded calls always pass 0 as worker_i.)
>
> Also, it worries me a bit that we iterate over the collection set for 
> each thread to find the start. Seems like it could be a bottle neck if 
> we have many threads and many regions in the collection set. We can 
> calculate the start regions by just iterating once over the collection 
> set since we know the number of threads. I am not sure where the best 
> place to add that logic is though. Maybe we could let 
> G1CollectorPolicy have a method that calculates the start regions once 
> for all worker threads. Then G1RemSet::scanRS() and 
> G1ParCompleteMarkInCSTask::work() could look up their start regions in 
> constant time.
>
>
> Bengt
>
>
>
> On 2011-11-22 18:23, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> I have a new webrev for this change based upon some review comments 
>> from Tony. The new webrev can be found at: 
>> http://cr.openjdk.java.net/~johnc/7112743/webrev.2/
>>
>> Thanks,
>>
>> JohnC
>>
>> On 11/18/11 10:18, John Cuthbertson wrote:
>>> Hi Everyone,
>>>
>>> Can I have a couple of volunteers look over these changes? The 
>>> webrev can be found at: 
>>> http://cr.openjdk.java.net/~johnc/7112743/webrev.1/
>>>
>>> Summary:
>>> We have a workload where the serial "complete marking in collection 
>>> set" closure was taking a realy long time and lengthening the pause 
>>> times. Since this code might be going away at some point (see CR 
>>> 6888336:  G1: avoid explicitly marking and pushing objects in 
>>> survivor spaces) it was decided that a short term solution would be 
>>> to parallelize the existing code and have the GC workers execute the 
>>> code rather than the VM thread. Also included are some minor tweaks 
>>> to the PrintGCDetails output to print out the mark closure time and 
>>> the collection set freeing time (since some of the original code was 
>>> moved there).
>>>
>>> Testing: GC test suite (with 12 GC threads, makeing threshold of 5%, 
>>> marking verification enabled, and heap verification after GC 
>>> enabled); GC test suite on my workstation (4 GC threads, same flags 
>>> as before); Kitchensink; specjbb2005 configured to have at leat 60% 
>>> heap utilization.
>>>
>>> Thanks,
>>>
>>> JohnC
>>
>