RFR(L): 6484965: G1: piggy-back liveness accounting phase on marking

[Bengt Rutisson]

Hi John,

Thanks for updating your fix! Looks good.

One quesiton:
In concurrentMark.cpp it seems to me that the 
VerifyLiveObjectDataHRClosure could get the same kind of messed up 
output that Tony just fixed with 7123165 for the VerifyLiveClosure in 
heapRegion.cpp. There are several workers simultaneously doing the 
verification, right? Is it worth adding the same kind of locking that 
Tony added?

Bengt

On 2011-12-20 20:16, John Cuthbertson wrote:
> Hi Bengt,
>
> As I mentioned earlier - thanks for the code review. I've applied your 
> suggestions, merged with the the latest changeset in 
> hsx/hotspot-gc/hotspot (resolving any conflicts), fixed the int <-> 
> size_t issue you also mentioned, and retested using the GC test suite. 
> A new webrev can be found at: 
> http://cr.openjdk.java.net/~johnc/6484965/webrev.2/
>
> Specific replies are inline.
>
> On 11/28/11 05:07, Bengt Rutisson wrote:
>>
>> John,
>>
>> A little late, but here are some comments on this webrev. I know you 
>> have some more improvements to this change coming, but overall I 
>> think it looks good. Most of my comments are just minor coding style 
>> comments.
>>
>> Bengt
>>
>> concurrentMark.hpp
>>
>> Rename ConcurrentMark::clear() to  ConcurrentMark::clear_mark() or 
>> ConcurrentMark::unmark()? The commment you added is definitely needed 
>> to understand what this method does. But it would be even better if 
>> it was possible to get that from the method name itself.
>
> Done.
>
>> It seems like everywhere we use count_marked_bytes_for(int worker_i) 
>> we almost directly use the array returned to index with the heap 
>> region that we are interested in. How about wrapping all of this is 
>> in something like count_set_marked_bytes_for(int worker_i, int 
>> hrs_index) and count_get_marked_bytes_for(int worker_i, int 
>> hrs_index) ? That way the data structure does not have to be exposed 
>> outside ConcurrentMark. It would mean that 
>> ConcurrentMark::count_region() would have to take a worker_i value 
>> instead of a marked_bytes_array.
>
> I did not do this. I embed the marked_bytes array for a worker into 
> the CMTask for that worker to save a de-reference. This was one of the 
> requests from the original code walk-through. Avoiding the 
> de-reference in the CMTask::do_marking_step() shaves a couple of 
> points off the marking time. I think your suggestion would reinstate 
> the de-reference again and we would lose those few percentage points 
> again.
>
>> If you don't agree with the suggestion above I would suggest to 
>> change the name from count_marked_bytes_for() to 
>> count_marked_bytes_array_for() since in every place that it is being 
>> called the resulting value is stored in a local variable called 
>> marked_bytes_array, which seems like a more informative name to me.
>
> Done. I agree - the new name sounds better.
>
>> I think this comment:
>>
>>   // As above - but we don't know the heap region containing the
>>   // object and so have to supply it.
>>   inline bool par_mark_and_count(oop obj, int worker_i);
>>
>> should be something like "we don't know the heap region containing 
>> the object so we will have to look it up".
>>
>> Same thing here:
>>
>>   // As above - but we don't have the heap region containing the
>>   // object, so we have to supply it.
>>   // Should *not* be called from parallel code.
>>   inline bool mark_and_count(oop obj);
>>
>>
>
> Comments were changed to:
>
>
>> concurrentMark.cpp
>>
>> Since you are changing CalcLiveObjectsClosure::doHeapRegion() anyway, 
>> could you please remove this unused code (1393-1397):
>>
>>         /*
>>         gclog_or_tty->print_cr("Setting bits from %d/%d.",
>>                                obj_card_num - _bottom_card_num,
>>                                obj_last_card_num - _bottom_card_num);
>>         */
>>
>>
>
> Done.
>
>> What about the destructor ConcurrentMark::~ConcurrentMark() ? I 
>> remember Tony mentioning that it won't be called. Do you still want 
>> to keep the code?
>
> I removed the entire destructor - I don't see it being called in the 
> experiments I've run.
>
>> FinalCountDataUpdateClosure::set_bit_for_region()
>> Probably not worth it, but would it make sense to add information in 
>> a startsHumongous HeapRegion to be able to give you the last 
>> continuesHumongous region? Since we know this when we set the regions 
>> up it seems like a waste to have to iterate over the region list to 
>> find it.
>
> If you read the original comment - the original author did not want to 
> make any assumptions about the internal field values of the 
> HeapRegions spanned by a humongous object and so used the loop 
> technique. I think you are correct and I now use the information in 
> the startsHumongous region to find the index of the last 
> continuesHumongous region spaned by the H-obj.
>
>> G1ParFinalCountTask
>> To me it is a bit surprising that we mix in the verify code inside 
>> this closure. Would it be possible to extract this code out somehow?
>
> I did it this way to avoid another iteration over the heap regions. 
> But it probably does make more sense to separate them and use another 
> iteration to do the verify. Done.
>
>> Line 3378: "// Use fill_to_bytes". Is this something you plan on doing?
>
> I removed the comment. I was thinking of doing this as fill_to_bytes 
> is typically implemented using (a possibly specialized version of) 
> memset. But it's probably not worth it in this case.
>
>> G1ParFinalCountTask::work()
>> Just for the record. I don't really like the way we have to set up 
>> both a VerifyLiveObjectDataHRClosure and a Mux2HRClosure even though 
>> we will only use them if we have VerifyDuringGC enabled. I realize it 
>> is due to the scoping, but I still think it obstucts the code flow 
>> and introduces unnecessary work. Unfortunately I don't have a good 
>> suggestion for how to work around it.
>>
>> Since both VerifyLiveObjectDataHRClosure and a Mux2HRClosure are 
>> StackObjs I assume it is not possible to get around the issue with a 
>> ResourceMark.
>
> Now that the verification is performed in a separate iteration of the 
> heap regions there's no need to create the 
> VerifyLiveObjectDataHRClosure  and Mux2HRClosure instances here. Done. 
> I have also removed the now-redundant Mux2HRClosure.
>
> Hopefully the new webrev addresses these comments.
>
> Thanks again for looking.
>
> JohnC
>