RFR (S): 7080389: G1: refactor marking code in evacuation pause copy closures
john.cuthbertson at oracle.com
Fri Aug 19 13:28:01 PDT 2011
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:
> 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
> * 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.
> 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
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.
> 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
>> 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.
More information about the hotspot-gc-dev