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

Jon Masamitsu jon.masamitsu at oracle.com
Fri Mar 4 23:44:58 UTC 2016

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.

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