RFR(S): 8149834: gc/shared/gcTimer.cpp:88 assert(_is_concurrent_phase_active) failed: A concurrent phase is not active

[sangheon]

Hi Jon,

Thanks for reviewing this!

On 03/04/2016 03:44 PM, Jon Masamitsu wrote:
> Sangheon,
>
> Change looks good.
>
> One  issue for you consideration.
>
> http://cr.openjdk.java.net/~sangheki/8149834/webrev.00/src/share/vm/gc/g1/g1ConcurrentMark.cpp.frames.html 
>
>
> 1042 void G1ConcurrentMark::register_concurrent_phase_end() {
> 1043 register_concurrent_phase_end_common(false);
> 1044 }
> 1045
> 1046 void G1ConcurrentMark::register_concurrent_gc_end() {
> 1047 register_concurrent_phase_end_common(true);
> 1048 }
>
> I understand that adding these functions is a style and
> I've used  that style before but in this case it confuses
> me.  Having register_concurrent_phase_end()
> call register_concurrent_phase_end_common() makes sense
> since their names are similar but I'm less clear
> about register_concurrent_gc_end() calling
> register_concurrent_phase_end_common().
>
> Maybe register_concurrent_gc_end() should be renamed
> register_concurrent_phase_end_and_stop_timer().
>
> Or have two statics
> static const bool StopTimer = true;
> static const bool DontStopTime = false;
>
> and then call
>
> register_concurrent_phase_end_common(StopTimer)
>
> register_concurrent_phase_end_common(DontStopTimer)
>
> You could drop the "_common" part of the name.
Agree.
I like first suggestion more.

Here's a new webrev:
http://cr.openjdk.java.net/~sangheki/8149834/webrev.01/
http://cr.openjdk.java.net/~sangheki/8149834/webrev.01_to_00

Thanks,
Sangheon


>
>
> As I said, it's a matter of style so you can choose.
>
> Jon
>
>
>
> On 03/02/2016 11:41 PM, sangheon wrote:
>> Hi all,
>>
>> Could I have a couple of reviews for this quick fix?
>>
>> There is a race between VMThread and ConcurrentMarkThread for 
>> ConcurrentGCTimer.
>> At the end of abort, VMThread is trying to end concurrent timer and 
>> if ConcurrentMarkThread is trying to start or end concurrent phase, 
>> timer related asserts will be fired.
>> (Only ConcurrentMarkThread starts a concurrent phase)
>> We have 3 different cases but the root cause is same[1].
>>
>> This proposal is introducing 3 phases of started, not started and 
>> stopping for concurrent phase status.
>> And the status is updated by cmpxchg.
>>
>> However, I think more proper fix would be eliminating the race.
>> Currently G1CollectedHeap has ConcurrentGCTimer but it is mostly used 
>> from ConcurrentMarkThread.
>> So moving the timer and related routines to ConcurrentMark seems better.
>> I filed a new RFE for this[2].
>>
>> Many thanks to Jon, Bengt(base patch as well) and StefanK for the 
>> discussion.
>>
>> CR: https://bugs.openjdk.java.net/browse/JDK-8151085
>> Webrev: http://cr.openjdk.java.net/~sangheki/8149834/webrev.00
>> Testing: JPRT, local test with adding some sleep at vm code.
>>
>> [1] Related bugs:
>> https://bugs.openjdk.java.net/browse/JDK-8145996
>> https://bugs.openjdk.java.net/browse/JDK-8150819
>>
>> [2] RFE for proper fix:
>> https://bugs.openjdk.java.net/browse/JDK-8151085
>>
>> Thanks,
>> Sangheon
>>
>>
>>
>>
>