RFR(L): 6484965: G1: piggy-back liveness accounting phase on marking
john.cuthbertson at oracle.com
Mon Nov 28 10:55:46 PST 2011
Thanks for the review. Inline....
On 11/28/11 05:07, Bengt Rutisson wrote:
> 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.
> 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.
> 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);
> 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);
> 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.
> 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.
> 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.
> 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
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.
More information about the hotspot-gc-dev