RFR: 8175375: MemoryPoolMXBean.getCollectionUsage() does not works with -XX:+UseG1GC -XX:+ExplicitGCInvokesConcurrent

[Yasumasa Suenaga]

Hi,

2018年9月6日(木) 23:01 Thomas Schatzl <thomas.schatzl at oracle.com>:
>
> Hi,
>
> On Thu, 2018-09-06 at 04:59 +0000, Fairoz Matte wrote:
> > Hi Thomas,
> >
> > Thanks for looking into this.
> >
> > > > [...]
> > > > JBS issue - https://bugs.openjdk.java.net/browse/JDK-8175375
> > > > Webrev - http://cr.openjdk.java.net/~fmatte/8175375/webrev.00/
> > >
> > >   I think this change breaks actual accounting: the
> > > G1MonitoringScopes will
> > > immediately be destructed at the end of the if/else blocks, so they
> > > will not
> > > be measuring values correctly.
> > >
> > > I think you need to set the full_gc parameter of the scope
> > > depending on the
> > > gc cause instead.
> >
> > This call is to only initialize G1MonitoringScopes, so when we call
> > G1MonitoringSupport::update_sizes() it still in same scoping level as
> > an active TraceMemoryManagerStats object.
> > Just forgot to mention that, I have added new test case
> > "TestTenuredGenPoolCollectionUsage.java", which gives expected
> > results before and after the patch.
>
> But the MonitoringScope also implicitly starts and ends the embedded
> TraceCollectorStats/TraceMemoryManagerStats, i.e. calls their
> destructors, which e.g. save some current time stamp. Which will be
> completely off compared to before.
>
> > But it is good idea to keep the scope depending on the gc cause. I
> > have updated the patch accordingly.
> > Please find the updated webrev -
> > http://cr.openjdk.java.net/~fmatte/8175375/webrev.01/
>
> The fix is good, but looking at the CR I am not sure that this fix
> addresses the entire problem, or I believe the problem has not been
> understood rightly.

IMHO we need to treat the following gc causes as same as _java_lang_system_gc:

    _full_gc_alot
    _jvmti_force_gc
    _heap_inspection
    _heap_dump
    _wb_full_gc
    _metadata_GC_threshold
    _metadata_GC_clear_soft_refs
    _dcmd_gc_run

> In this case the user wanted to have the old pool MemoryMXBean updated
> with the concurrent start gc. I assume that person is moving from CMS
> noting this inconsistency. CMS seems to update the old pool for any
> concurrent start gc.
>
> So this change only fixes the case when calling system.gc(), not any
> other start of marking gc. (Probably the predicate collector_state()-
> >in_initial_mark_gc() is correct here?)
>
> I believe further investigation of what MXBeans/pool(s)/jstat counters
> CMS updates in any initial mark GC would be warranted to get expected
> behavior, and fix this appropriately.
>
> I personally do not have an opinion, as an concurrent start could be
> considered both part of a "young" and "old" gc. Actually I would be
> really happy if somebody wrote down the expectations of what
> MemoryMXBean, pools and the CollectionCounters (jstat?) should be
> affected when (in form of a unit test preferably).
>
> Because changing this "full_gc" parameter depending on type of young gc
> or gc cause of the G1MonitoringScope also has the subtle effect that
> instead of the "young" collection counters
> (G1MonitoringSupport::_incremental_collection_counters) instead of the
> "old" collection counters
> (G1MonitoringSupport::_full_collection_counters) are updated. Instead
> of always the former.
>
> Cc'ed phh and ysuenaga because they seem to be users of these :)

I added _cgc_counters to record STW phases in concurrent GC in JDK-8153333.
I think this is not a bug because Full GC is invoked as concurrent GC
if you pass -XX:+ExplicitGCInvokesConcurrent to commandline arguments.
So it should be recorded in _cgc_counters.

In case of CMS, it has two STW phase: Initial-Mark and Remark. But
they just mark live objects only. So usage is not be changed.
In case of G1, Remark and Cleanup are STW phases, they are also
recorded in _cgc_counters.


Thanks,

Yasumasa


> Thanks,
>   Thomas
>