RFR(M): 8151085: Change G1 concurrent timer and tracer measuring time

[Bengt Rutisson]

Hi Sangheon,

I like this cleanup. I think it makes things much more clear.

I'm fine with the behavioral changes regarding the timing. Thanks for 
listing them clearly in the table.

One minor nit:

g1CollectedHeap.cpp

2800   return G1HeapSummary(heap_summary, (Heap_lock->owned_by_self() ? 
used() : used_unlocked()),
2801                        eden_used_bytes, eden_capacity_bytes, 
survivor_used_bytes, num_regions());

I would prefer to store (Heap_lock->owned_by_self() ? used() : 
used_unlocked()) in a local variable, just like eden_used_bytes, 
eden_capacity_bytes and survivor_used_bytes.

Other than that it looks good to me.

Thanks,
Bengt

On 2016-03-11 20:48, sangheon wrote:
> Hi all,
>
> Could I have some reviews for this change?
>
> Concurrent gc timer and tracer are mainly used from concurrent mark 
> thread except when abort happens. This abort results in a race 
> condition between VM thread and concurrent mark thread.
> So I propose to do timer and tracer related works only from concurrent 
> mark thread.
>
> Measurements are changed like below:
>
> 	Current
> 	Proposal
> Start timer and tracer
> 	When Young Initial Mark GC starts from VM thread
> 	When actually starts concurrent cycle from concurrent mark thread
> End timer and tracer
> 	1. At the end of concurrent cycle
> 2. When abort happens 	At the end of concurrent cycle
>
> * I think this will be okay as even though we abort, we continue 
> working for concurrent stuffs to finish concurrent work cycle. And we 
> were just not measuring them.
> Heap summary (before-gc)
> 	At the beginning of Young Initial Mark GC.
> But concurrent mark thread is not working at that time. 	At the start 
> of concurrent cycle.
> Heap summary (after-gc)
> 	1. When abort happens before cleanup: sent when abort happens.
> 2. When abort happens after cleanup or normal case(w/o abort): sent at 
> the end of cleanup. 	At the end of concurrent cycle which means it 
> will include all minor clearing after 'Pause Cleanup'.
>
>
> This proposal also includes related changes like below:
> 1. Removed the patch for JDK-8149834(race between VM thread and 
> concurrent mark thread for concurrent gc timer): 8149834 was making 
> related functions atomically while this proposal is changing not to 
> happen that race.
> 2. Removed G1CollectorState::_concurrent_cycle_started: no longer 
> needed as related logics are running from concurrent mark thread.
>
> Currently 'total concurrent time' is not same as 'sum of concurrent 
> phases' and the reason is that the concurrent timer is started when 
> young initial mark gc happens. Secondly, concurrent mark thread is not 
> counting the time for waiting VMThread to finish VM 
> operation(SuspendibleThreadSet).
> I am planning to enhance this after '8151614: Improve logging in 
> concurrent mark code' is pushed.
>
> CR: https://bugs.openjdk.java.net/browse/JDK-8151085
> Webrev: http://cr.openjdk.java.net/~sangheki/8151085/webrev.00
> Testing: JPRT, RBT(hotspot_all, testlist: running now), local 
> reproducer for JDK-8149834
>
> Thanks,
> Sangheon

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160314/9028ddf6/attachment.htm>