8042933: assert(capacity_until_gc >= committed_bytes) failed

[Erik Helin]

Hi StefanK,

thanks for reviewing! Please see new patches at:

Incremental: http://cr.openjdk.java.net/~ehelin/8042933/webrev.00-01/
Full: http://cr.openjdk.java.net/~ehelin/8042933/webrev.01/

and comments inline.

On Wednesday 28 May 2014 13:56:48 PM Stefan Karlsson wrote:
> On 2014-05-28 13:10, Erik Helin wrote:
> > Hi all,
> > 
> > this is the second attemp to fix JDK-8034852 [0], but since I backed
> > out the previous attempt, I have to use a new bug [1] (you can't use
> > the same bug-id for two different committs).
> > 
> > For a background to the original problem, please see the old email
> > thread [2]. This patch is just and updated version of the original
> > patch [3]. The issue with the original path is that
> > MetaspaceGC::allowed_expansion can, during special circumstances,
> > allow the Metaspace high-water mark (HWM aka
> > MetaspaceGC::_capacity_until_GC) to become larger than the amount of
> > committed memory. Keeping the HWM less than or equal to how much we
> > have committed is an invariant we would like to be true in Metaspace
> > at all times to make it easier to reason about the sizing heuristics.
> > 
> > This patch changes MetaspaceGC::allowed_expansion to always enforce
> > the invariant that HWM <= committed_bytes. However, we still want to
> > allow the VM to allocate as much metadata it wants during
> > initialization. To achieve this, we set the HWM to MaxMetaspaceSize
> > when initializing Metaspace and then reset the HWM to
> > max(MetaspaceSize, HWM) once the initialization is done. Thanks
> > StefanK for coming up with this approach!
> > 
> > MetaspaceGC::allowed_expansion previously also allowed the VM to
> > allocate as much metadata it wants when the GC lock is active and we
> > are awaiting a GC. The new behaviour will be to try to expand the HWM
> > and then allocate (the expand_and_allocate call in
> > CollectorPolicy::satisfy_failed_metadata_allocation). Under very
> > special circumstances this might cause a thread to now await the GC in
> > GC_locker::stall_until_clear instead of getting its metadata and
> > continue executing.
> > 
> > Webrev:
> > http://cr.openjdk.java.net/~ehelin/8042933/webrev.00/
> 
> http://cr.openjdk.java.net/~ehelin/8042933/webrev.00/src/share/vm/memory/met
> aspace.hpp.udiff.html
> 
> Could you add a comment explaining why we use MaxMetaspaceSize here,
> instead of the MetaspaceSize:

Sure, done.

> -  static void initialize() { _capacity_until_GC = MetaspaceSize; }
> +  static void initialize() { _capacity_until_GC = MaxMetaspaceSize; }
> +  static void post_initialize();
> 
> Maybe it would be better to move the implementation of initialize() to
> metaspace.cpp and place it together with the post_initialize() function?
> I think that would make it more apparent that we just
> setcapacity_until_GC temporarily to MaxMetaspaceSize.

Agree, fixed.

> http://cr.openjdk.java.net/~ehelin/8042933/webrev.00/src/share/vm/runtime/th
> read.cpp.udiff.html
> 
> Could add a blank line between these two functions, so that they are not
> grouped together?

Yep, updated.

Thanks,
Erik

>     // Set flag that basic initialization has completed. Used by exceptions
> and various // debug stuff, that does not work until all basic classes have
> been initialized. set_init_completed();
> +  Metaspace::post_initialize();
> 
> Otherwise, this looks good.
> 
> thanks,
> StefanK
> 
> > Testing:
> > - JPRT
> > 
> > - Ad-hoc testing (on all platforms):
> >    - Kitchensink
> >    - runThese
> >    - Parallel Class Loading testlist
> >    - Metaspace testlist
> >    - GC nightly testlist
> >    - OOM testlist
> >    - Quick testlist
> >    - JTREG tests in both VM and JDK
> > 
> > Thanks,
> > Erik
> > 
> > [0]: https://bugs.openjdk.java.net/browse/JDK-8034852
> > [1]: https://bugs.openjdk.java.net/browse/JDK-8042933
> > [2]:
> > http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-April/013716.html
> > [3]: http://cr.openjdk.java.net/~ehelin/8034852/webrev.00/