RFR (M): 8019342: G1: High "Other" time most likely due to card redirtying

[Thomas Schatzl]

Hi,

On Mon, 2014-04-14 at 15:27 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> 
> Looks good.
> 
> A couple of minor things:
> 
> You made apply_closure_to_all_completed_buffers() and 
> par_apply_closure_to_all_completed_buffers() take the closure they 
> should apply as a parameter instead of looking it up in the _closure 
> instance variable. I like this a lot better, but why did you keep the 
> _closure instance variable for other methods 
> (iterate_closure_all_threads() and mut_process_buffer()). It would have 
> been nice to have the same pattern for all methods. If it is difficult 
> to change the others to take a closure as parameter I think I would 
> prefer to revert the apply methods to also use the instance variable.

I fixed that by making all methods use a closure parameter.
Mut_process_buffer() still uses the "internal" closure (renamed
_mut_process_closure) as it is called by the Java threads in a static
context, so you cannot really avoid having this member around.

_mut_process_closure is also passed in the DCQS constructor, and cannot
be set any more.

> 
> Not strictly related to your change, but it would make it easier to 
> understand you change:
> In G1CollectedHeap::check_ct_logs_at_safepoint() we start out by doing:
>    DirtyCardQueueSet& dcqs = JavaThread::dirty_card_queue_set();
> But in that method we still use JavaThread::dirty_card_queue_set() 
> explicitly twice. It would be nice to replace these calls with dcqs to 
> make it clear that it is the same instance.

Done.

> 
> These two formatting changes are unrelated to your other changes:
> 
> dirtyCardQueue.cpp:  165 BufferNode* 
> DirtyCardQueueSet::get_completed_buffer(int stop_at) {
> g1CollectedHeap.cpp:  173  YoungList::YoungList(G1CollectedHeap* g1h) :
> 
> I would prefer to leave those out.

Done.

New webrev at:
http://cr.openjdk.java.net/~tschatzl/8019342/webrev.1

Diff to previous (containing the changes due to the closure passing):
http://cr.openjdk.java.net/~tschatzl/8019342/webrev.1.diff

Testing:
jprt

Thanks,
  THomas