8042933: assert(capacity_until_gc >= committed_bytes) failed

[Stefan Karlsson]

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/metaspace.hpp.udiff.html

Could you add a comment explaining why we use MaxMetaspaceSize here, 
instead of the MetaspaceSize:

-  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.

http://cr.openjdk.java.net/~ehelin/8042933/webrev.00/src/share/vm/runtime/thread.cpp.udiff.html

Could add a blank line between these two functions, so that they are not 
grouped together?

    // 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/
>