8042933: assert(capacity_until_gc >= committed_bytes) failed

[Stefan Karlsson]

On 2014-05-29 14:43, Erik Helin wrote:
> Hi StefanK,
>
> thanks for reviewing! Please see new patches at:
>
> Incremental: http://cr.openjdk.java.net/~ehelin/8042933/webrev.00-01/

Looks good.

StefanK

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