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

[John Cuthbertson]

Hi Bengt,

Thanks for the review. 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.

Sure. OK.

> 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.

OK. Good idea.

> 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.

I think I'll do the renaming anyway - in addition to the suggestion above.

> 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);

OK.

> 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);
>         */
>
>

OK.

> What about the destructor ConcurrentMark::~ConcurrentMark() ? I 
> remember Tony mentioning that it won't be called. Do you still want to 
> keep the code?

Good point. I still need to run the experiment to see if any of the code 
in the destructor is needed.

> 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.

I think we have the information - we can get the "raw" heap region 
containing "end()". That should be the last ContinuesHumongous region in 
the series. Excellent idea.

> 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?

It should be straight-forward to do. Doing so, however, would mean 
another iteration of the heap regions (as well as another wrapper task 
if we want to do it non-serially).

> Line 3378: "// Use fill_to_bytes". Is this something you plan on doing?

I believe it was but can't recall why I didn't. I either change the code 
or remove the comment.

> 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.

If we separate out the verify code from the G1ParFinalCountTask then 
this should go away.

> Since both VerifyLiveObjectDataHRClosure and a Mux2HRClosure are 
> StackObjs I assume it is not possible to get around the issue with a 
> ResourceMark.

I'm not sure I get what you mean. That they are allocated on stack 
should mean that we don't need to reclaim resource memory and so no need 
for a ResourceMark. Can you clarify a bit?

Thanks again for the review.

JohnC