RFR: 8241160: Concurrent class unloading reports GCTraceTime events as JFR pause sub-phase events

[Stefan Karlsson]

On 2020-03-25 23:37, Kim Barrett wrote:
>> On Mar 19, 2020, at 5:44 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>
>> Hi all,
>>
>> Please review this patch to rewrite the GCTimer, and associated classes, to not allow nested phases of different types (pause or concurrent).
>>
>> https://cr.openjdk.java.net/~stefank/8241160/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8241160
> Looks good.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/gcTimer.cpp
>   105 GCPhase::PhaseType TimePartitions::current_phase_type() const {
>   106   int level = _active_phases.count();
>   107   int index = _active_phases.phase_index(level - 1);
>
> Maybe assert level > 0.  No need for a new webrev if you want to make
> this change.

I'll add the assert.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/shared/gcTraceSend.cpp
>   306     assert(phase->level() < 2, "There is only two levels for ConcurrentPhase");
>
> Seems like there ought to be a named constant for that "2", similar to
> PhasesStack::PHASE_LEVELS used in a similar place in visit_pause. But
> then, there's a mismatch between PHASE_LEVELS (6) and the number of
> cases in visit_pause (4). That's "interesting"...

Yeah. visit_pause checks that we don't push too many levels of phases, 
but we already check that when we push phases:

void PhasesStack::push(int phase_index) {
   assert(_next_phase_level < PHASE_LEVELS, "Overflow");

...
   int level = _active_phases.count();

   GCPhase phase;
   phase.set_type(type);
   phase.set_level(level);
   phase.set_name(name);
   phase.set_start(time);

   int index = _phases->append(phase);

   _active_phases.push(index);

I think it would make sense to remove that assert, to get rid of that 
confusion.

I'm not so sure about removing 'assert(phase->level() < 2' since it's 
there to to catch when we start to use deeper nesting of the concurrent 
phases. Maybe if we also add EventGCPhaseConcurrentLevel2-4 (analogous 
to visit_pause) and then get rid of this assert?

If we want to go ahead and do any of these changes (find a name for the 
2 or adding events) I'll create a separate RFR.

Thanks,
StefanK

> ------------------------------------------------------------------------------
>