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

[Bengt Rutisson]

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
>