RFR (S): 7080389: G1: refactor marking code in evacuation pause copy closures

[John Cuthbertson]

Hi Bengt,

Thanks for the review - comments are inline....

On 08/19/11 06:01, Bengt Rutisson wrote:
>
> Hi John
>
> Looks good to me.
>
> A couple of minor comments:
>
> g1CollectedHeap.cpp:
>
> In the do_oop_work method (lines 4324-4362):
>
> * To show the limited scope of the should_mark variable I would like 
> to move the declaration of should_mark to just before it is being used 
> at line 4349.
Done! Tony also suggested this.
>
> * At the two places in the do_oop_work method where you use the 
> do_mark_forwardee paramter you have comments saying that it is an 
> initial mark closure. This is correct, so I think that I would 
> actually prefer that the parameter was called something with inital 
> mark rather than having to have the comments.
OK. I have changed the comments to read "... we're a root scanning 
closure during and initial-mark pause (i.e. do_mark_object will be 
true)....".
>
> * On the dead code subject: this closure seems to be unused (in 
> g1CollectedHeap.cpp): "G1ParScanAndMarkHeapRSClosure   
> scan_mark_heap_rs_cl(_g1h, &pss);"
I have this (and other clean ups in g1OopClosures.hpp) in the reference 
processing webrev - I'd rather not duplicate it here.
>
> g1OopClosures.hpp:
>
> Just a nitpick: You removed a line break at row 114 and added a line 
> break at row 122-123. Since you didn't change anything else on these 
> lines it would make the diff easier to view if you left those changes 
> out.
>
To me, it's difficult to pick out the template parameters - especially 
when one is on a line on it's own. But I'll back them out.

JohnC
> Bengt
>
> On 2011-08-18 20:17, John Cuthbertson wrote:
>> Hi Everyone,
>>
>> Can I have a couple of volunteers review these refactoring changes to 
>> the marking code used during evacuation pauses (both initial mark 
>> pauses and regular evacuation pauses when marking is active) - the 
>> change can be found at 
>> http://cr.openjdk.java.net/~johnc/7080389/webrev.0/.
>>
>> The refactoring changes fix an issue that was seen with the code 
>> changes for 6486945.
>>
>> During an initial mark pause, during root scanning, one thread had 
>> successfully forwarded an object and had started to copy it. While 
>> the object was being copied to its new location, another thread saw 
>> that the object had been forwarded and, after checking that the new 
>> location was unmarked, successfully marked the new location. The 
>> first thread would finish the copying, see that the new location was 
>> marked and skip the mark. The situation I ran into was that I was 
>> attempting to obtain the size of the new object just after it was 
>> marked (by the thread doing the marking) and the old object had not 
>> yet been fully copied to its new location.
>>
>> With these refactoring changes, the thread that successfully forwards 
>> an object in the collection set will mark the forwardee after copying 
>> - allowing me to safely obtain it's size.
>>
>> Testing: several runs of the GC test suite with a marking threshold 
>> of 10 and 20%, Kitchensink, and jprt.
>>
>> Thanks,
>>
>> JohnC
>>
>>
>