[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR (M): 8019342: G1: High "Other" time most likely due to card redirtying
From:       Bengt Rutisson <bengt.rutisson () oracle ! com>
Date:       2014-04-15 11:55:12
Message-ID: 534D1E20.6090506 () oracle ! com
[Download RAW message or body]


Hi Thomas,

Thanks for doing these cleanups!

Looks good!
Bengt


On 2014-04-15 12:06, Thomas Schatzl wrote:
> 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
>
>
>

[prev in list] [next in list] [prev in thread] [next in thread]