RFR (M): 8207200: Committed > max memory usage when getting MemoryUsage

[Thomas Schatzl]

Hi Sangheon,

On Fri, 2018-08-24 at 11:42 -0700, sangheon.kim at oracle.com wrote:
> Hi Thomas,
> 
> On 8/24/18 3:17 AM, Thomas Schatzl wrote:
> > Hi,
> > 
> >    ping! :)
> > 
> > On Wed, 2018-08-08 at 17:15 +0200, Thomas Schatzl wrote:
> > > Hi all,
> > > 
> > >    can I have reviews for this change that fixes some races when
> > > reading from G1MonitoringSupport members, causing some assertion
> > > failing after weeks of running some programs that poll the G1
> > > MemoryPools.
> > > 
> > > It does so by adding a mutex that is held during reading and
> > > writing them.
> > > 
> > > There is also a new explicit call in CollectedHeap to get a
> > > consistent overall MemoryUsage information instead of the
> > > existing code looping over the heap's memory pools. That one is
> > > problematic too otherwise.
> > > 
> > > There is still no change in the calculation of the values of the
> > > MemoryUsage members.
> > > 
> > > CR:
> > > https://bugs.openjdk.java.net/browse/JDK-8207200
> > > Webrev:
> > > http://cr.openjdk.java.net/~tschatzl/8207200/webrev
> > > Testing:
> > > hs-tier1-4,jdk-tier1-3
> > > 
> > > This change is based on the changes for JDK-8209062.
> > 
> >    these changes have been pushed by now.
> 
> Looks good as is.
> 
> One minor nit(you can skip) at 
> src/hotspot/share/gc/g1/g1MonitoringSupport.cpp.
> As you are repeating below pattern, you can make a common method and
> use 
> it for 4 locations.
> G1MonitoringSupport::memory_usage(), eden_space_memory_usage(), 
> survivor_space_memory_usage() and
> old_space_memory_usage().
> 
> MemoryUsage memory_usage_locked(size_t initial, used, committed, max)
> {
>      MutexLockerEx x(&_update_mutex,
> Mutex::_no_safepoint_check_flag);
>      return MemoryUsage(initial, used, committed, max);
> }

  this won't work as we need to make sure that the values copied to
MemoryUsage (initial, used, committed, max) are read within the scope
of the Mutex. Otherwise they may again be inconsistent.

> BTW, how did you verify this patch? Or have you test this patch with
> the reporter?
> I think this patch will fix the problem, but just curious.

There has been no extended testing as I never got to reproduce it
locally. In hindsight the problem and its fix are "obvious" though.

Thanks for your review,
  Thomas