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

[Fairoz Matte]

Hi Paul, Yasumasa and Thomas,

By now I am convinced that it is not an issue.
Thanks for providing detailed analysis on this.

Closing 8175375 as not an issue.

Thanks,
Fairoz

> -----Original Message-----
> From: Yasumasa Suenaga <yasuenag at gmail.com>
> Sent: Friday, September 07, 2018 7:10 AM
> To: Thomas Schatzl <thomas.schatzl at oracle.com>
> Cc: Fairoz Matte <fairoz.matte at oracle.com>; hotspot-gc-dev <hotspot-gc-
> dev at openjdk.java.net>; hohensee at amazon.com
> Subject: Re: RFR: 8175375: MemoryPoolMXBean.getCollectionUsage() does
> not works with -XX:+UseG1GC -XX:+ExplicitGCInvokesConcurrent
> 
> 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
> >