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

[John Cuthbertson]

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