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

sangheon sangheon.kim at oracle.com
Sat Mar 5 00:50:30 UTC 2016

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::registerconcurrentphaseend() { 1043 registerconcurrentphaseendcommon(false); 1044 } 1045 1046 void G1ConcurrentMark::registerconcurrentgcend() { 1047 registerconcurrentphaseendcommon(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 registerconcurrentphaseend() call registerconcurrentphaseendcommon() makes sense since their names are similar but I'm less clear about registerconcurrentgcend() calling registerconcurrentphaseendcommon(). Maybe registerconcurrentgcend() should be renamed registerconcurrentphaseendandstoptimer(). Or have two statics static const bool StopTimer = true; static const bool DontStopTime = false; and then call registerconcurrentphaseendcommon(StopTimer) registerconcurrentphaseendcommon(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

More information about the hotspot-gc-dev mailing list