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

[Hohensee, Paul]

Also, forgot to note that there's a jstat counter for concurrent collection cycles, see in g1MonitoringSupport.hpp "CollectorCounters*   _conc_collection_counters;" and associated code.

On 9/6/18, 10:59 AM, "hotspot-gc-dev on behalf of Hohensee, Paul" <hotspot-gc-dev-bounces at openjdk.java.net on behalf of hohensee at amazon.com> wrote:

    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