RFR(S): 8149834: gc/shared/gcTimer.cpp:88 assert(_is_concurrent_phase_active) failed: A concurrent phase is not active
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