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

[Hohensee, Paul]

I don't think this is a bug. It makes perfect sense to me that the G1 Old Gen pool may not be updated by a concurrent cycle. If the heap is big enough (and your test doesn't set -Xmx, so it is), there will be no mixed collection, so nothing will be promoted to the old gen, so the G1 Old Gen pool will correctly read as empty. Run with GC logging enabled to see if that's what's happening.

I suggest writing a test that uses the white box API and a small -Xmx to force a mixed collection (see test/hotspot/jtreg/gc/g1/mixedgc/TestOldGenCollectionUsage.java for an example). You should see that the G1 Old Gen pool is updated after a concurrent cycle if a mixed collection occurs. The fix for https://bugs.openjdk.java.net/browse/JDK-8195115 made the G1 Old Gen pool correctly reflect the result of a mixed collection.

I'm in the process of modifying the patch for https://bugs.openjdk.java.net/browse/JDK-8196989 to fit with Thomas' recent G1 serviceability refactoring. It details the way things will work once the patch is done.

Thanks,

Paul

On 9/6/18, 7:02 AM, "Thomas Schatzl" <thomas.schatzl at oracle.com> wrote:

    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.
    
    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 :)
    
    Thanks,
      Thomas