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

[Jon Masamitsu]

On 03/04/2016 04:50 PM, sangheon wrote:
> 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 for the change.  Looks good.

Jon

>
> 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
>>>
>>>
>>>
>>>
>>
>