RFR: JDK-8087291: InitialBootClassLoaderMetaspaceSize and CompressedClassSpaceSize should be checked consistent from MaxMetaspaceSize
David Holmes
david.holmes at oracle.com
Thu Oct 12 04:22:29 UTC 2017
Hi Yasumasa,
On 12/10/2017 2:10 PM, Yasumasa Suenaga wrote:
> Hi all,
>
> This change was tried to push via JPRT, but it failed because
> test/hotspot/jtreg/runtime/SharedArchiveFile/MaxMetaspaceSizeTest.java
> was failed.
> Also I heard the comment that the changes in arguments.cpp which set
> ergonomic value to CompressedClassSpaceSize should be moved to
> Metaspace::ergo_initialize().
>
> I uploaded new webrev. Could you review again?
> This webrev works fine on test/hotspot/jtreg/runtime/Metaspace and
> test/hotspot/jtreg/runtime/SharedArchiveFile.
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.06/
I'll let Coleen cover the technical details, I'll just pick up a few
style nits:
3328 /* Initial virtual space size will be calculated at
global_initialize() */
Use // comment
3329 size_t min_metaspace_sz =
3330 VIRTUALSPACEMULTIPLIER *
InitialBootClassLoaderMetaspaceSize;
^ should indent only to here
3336 FLAG_SET_ERGO(size_t, CompressedClassSpaceSize,
3337 MaxMetaspaceSize -
min_metaspace_sz);
^ should indent only to here
3341 FLAG_SET_ERGO(size_t, InitialBootClassLoaderMetaspaceSize,
3342
min_metaspace_sz);
^ should indent only to here
You may have time to fix these in place before anyone else sees the
webrev :)
Thanks,
David
-----
>
> Thanks,
>
> Yasumasa
>
>
>
> 2017-10-10 21:02 GMT+09:00 <coleen.phillimore at oracle.com>:
>> Hi Yasumasa, I like this better. I will sponsor it. I think it only needs
>> one reviewer, unless there were more? Please send me the result of hg
>> export.
>> thanks,
>> Coleen
>>
>>
>> On 10/9/17 11:09 PM, Yasumasa Suenaga wrote:
>>>
>>> Hi Coleen,
>>>
>>>>> I will sponsor this for you.
>>>
>>> Thanks!
>>>
>>>
>>>>> Hi, this looks reasonable but I would prefer that the code in
>>>>> arguments.cpp call something in metaspace.cpp, like
>>>>> check_metaspace_sizes()
>>>>> so that you don't have to tell arguments what the MediumChunk size is.
>>>
>>> I added new function calculate_min_metaspace_size() in metaspace.cpp
>>> to get minimum metaspace size, and uses return value from this
>>> function in metaspace initialization and metaspace size check.
>>>
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.05/
>>>
>>> Could you review again?
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> 2017-10-10 6:07 GMT+09:00 Man Cao <manc at google.com>:
>>>>
>>>> Thanks for you both updating and sponsoring the webrev!
>>>> We plan to back-port it to our JDK9 once it is submitted.
>>>>
>>>> -Man
>>>>
>>>> On Mon, Oct 9, 2017 at 6:15 AM, <coleen.phillimore at oracle.com> wrote:
>>>>>
>>>>> Hi, this looks reasonable but I would prefer that the code in
>>>>> arguments.cpp call something in metaspace.cpp, like
>>>>> check_metaspace_sizes()
>>>>> so that you don't have to tell arguments what the MediumChunk size is.
>>>>>
>>>>> I will sponsor this for you.
>>>>>
>>>>> thanks,
>>>>> Coleen
>>>>>
>>>>>
>>>>> On 9/26/17 11:05 PM, Yasumasa Suenaga wrote:
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> I added a testcase for this issue in new webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.04/
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>>
>>>>>> 2017-09-26 18:36 GMT+09:00 Yasumasa Suenaga <yasuenag at gmail.com>:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I uploaded webrev for this issue against jdk10/hs.
>>>>>>> Could you review it?
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.03/
>>>>>>>
>>>>>>>
>>>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 2017-09-21 3:11 GMT+09:00 Man Cao <manc at google.com>:
>>>>>>>>
>>>>>>>> Thank Yasumasa and Stefan for the responses.
>>>>>>>>
>>>>>>>> Good to know that the patch is not blocked due to breaking some
>>>>>>>> internal
>>>>>>>> invariants/assumptions, but just due to its P5 status.
>>>>>>>> Is it possible to push it to P4?
>>>>>>>>
>>>>>>>> -Man
>>>>>>>>
>>>>>>>> On Wed, Sep 20, 2017 at 5:16 AM, Yasumasa Suenaga
>>>>>>>> <yasuenag at gmail.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> (CC'ed hotspot-runtime-dev)
>>>>>>>>>
>>>>>>>>>> I think the reason is that this bug is a P5. The compressed class
>>>>>>>>>> space
>>>>>>>>>> belongs to the runtime code, so you might get more traction for
>>>>>>>>>> this
>>>>>>>>>> on the
>>>>>>>>>> hotspot-runtime-dev list.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I will send review request against jdk10/master or jdk10/hs after
>>>>>>>>> repos
>>>>>>>>> are opened.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2017/09/20 20:53, Stefan Karlsson wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Man,
>>>>>>>>>>
>>>>>>>>>> On 2017-09-13 20:55, Man Cao wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Yasumasa, Stefan,
>>>>>>>>>>>
>>>>>>>>>>> Do you have any thoughts on why this patch has been pending for 2+
>>>>>>>>>>> years? This patch could really save us from some annoying issues
>>>>>>>>>>> since we
>>>>>>>>>>> are automatically monitoring hsperfdata counters.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I think the reason is that this bug is a P5. The compressed class
>>>>>>>>>> space
>>>>>>>>>> belongs to the runtime code, so you might get more traction for
>>>>>>>>>> this
>>>>>>>>>> on the
>>>>>>>>>> hotspot-runtime-dev list.
>>>>>>>>>>
>>>>>>>>>> StefanK
>>>>>>>>>>
>>>>>>>>>>> -Man
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Aug 21, 2017 at 3:46 PM, Man Cao <manc at google.com
>>>>>>>>>>> <mailto:manc at google.com>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> I wonder if there is any recent update on the patch for
>>>>>>>>>>> JDK-8087291.
>>>>>>>>>>> Is it possible to push this patch into JDK9? Except for its
>>>>>>>>>>> low
>>>>>>>>>>> priority (P5),
>>>>>>>>>>> is there any complication that prevents this patch getting
>>>>>>>>>>> approved
>>>>>>>>>>> (for example, some JVM logic requires
>>>>>>>>>>> CompressedClassSpaceSize
>>>>>>>>>>> to be
>>>>>>>>>>> 1GB by default)?
>>>>>>>>>>>
>>>>>>>>>>> I work in the Java Platform Team at Google. We have
>>>>>>>>>>> encountered
>>>>>>>>>>> annoying issues that the hsperfdata counter
>>>>>>>>>>> "sun_gc_metaspace_maxCapacity" reporting
>>>>>>>>>>> a too large value (about 1GB) even if user sets
>>>>>>>>>>> -XX:MaxMetaspaceSize=100m, as well as GC log shows the
>>>>>>>>>>> confusing 1GB
>>>>>>>>>>> memory reserved by metaspace,
>>>>>>>>>>> regardless of MaxMetaspaceSize value. The root cause for
>>>>>>>>>>> these
>>>>>>>>>>> issues is that CompressedClassSpaceSize is not automatically
>>>>>>>>>>> capped
>>>>>>>>>>> by MaxMetaspaceSize
>>>>>>>>>>> during VM initialization, and this patch seems fix the root
>>>>>>>>>>> cause.
>>>>>>>>>>> (I'm aware that even after this patch, the reserved size
>>>>>>>>>>> could
>>>>>>>>>>> still
>>>>>>>>>>> be up to 2*MaxMetaspaceSize,
>>>>>>>>>>> but it is better than the current situation.)
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Man
>>>>>>>>>>>
>>>>>>>>>>> On 6/19/2015 00:34, Yasumasa Suenaga wrote:
>>>>>>>>>>>
>>>>>>>>>>> Thank you for your comment!
>>>>>>>>>>> > Try running a debug JVM with your patch with this
>>>>>>>>>>> command
>>>>>>>>>>> line.
>>>>>>>>>>> >
>>>>>>>>>>> > java -XX:MaxMetaspaceSize=4195328 -version
>>>>>>>>>>> Sorry, I've fixed it and uploaded webrev:
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.02/
>>>>>>>>>>>
>>>>>>>>>>> <http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.02/>
>>>>>>>>>>> It works on fastdebug VM.
>>>>>>>>>>> Please review again.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Yasumasa
>>>>>>>>>>>
>>>>>>>>>>> On 2015/06/18 10:45, Jon Masamitsu wrote:
>>>>>>>>>>> > Yasumasa,
>>>>>>>>>>> >
>>>>>>>>>>> > Try running a debug JVM with your patch with this
>>>>>>>>>>> command
>>>>>>>>>>> line.
>>>>>>>>>>> >
>>>>>>>>>>> > java -XX:MaxMetaspaceSize=4195328 -version
>>>>>>>>>>> >
>>>>>>>>>>> > On a linux system I get this when I build with your
>>>>>>>>>>> patch.
>>>>>>>>>>> >
>>>>>>>>>>> >> java -XX:MaxMetaspaceSize=4195328 -version
>>>>>>>>>>> >> # To suppress the following error report, specify
>>>>>>>>>>> this
>>>>>>>>>>> argument
>>>>>>>>>>> >> # after -XX: or in .hotspotrc:
>>>>>>>>>>> SuppressErrorAt=/metaspace.cpp:2324
>>>>>>>>>>> >> #
>>>>>>>>>>> >> # A fatal error has been detected by the Java
>>>>>>>>>>> Runtime
>>>>>>>>>>> Environment:
>>>>>>>>>>> >> #
>>>>>>>>>>> >> # Internal Error
>>>>>>>>>>> >>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> (/export/jmasa/java/jdk9-gc-code_review/src/share/vm/memory/metaspace.cpp:2324),
>>>>>>>>>>> >> pid=19099, tid=0x00007ff4b9b92700
>>>>>>>>>>> >> # assert(size > MediumChunk || size >
>>>>>>>>>>> ClassMediumChunk)
>>>>>>>>>>> failed: Not a
>>>>>>>>>>> >> humongous chunk
>>>>>>>>>>> >> #
>>>>>>>>>>> >
>>>>>>>>>>> >
>>>>>>>>>>> > Jon
>>>>>>>>>>> >
>>>>>>>>>>> >
>>>>>>>>>>> > On 6/17/2015 7:54 AM, Yasumasa Suenaga wrote:
>>>>>>>>>>> >> I want to continue to discuss about
>>>>>>>>>>> CompressedClassSpace and
>>>>>>>>>>> MaxMetaspace in this (RFR) thread.
>>>>>>>>>>> >>
>>>>>>>>>>> >>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-June/013873.html
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-June/013873.html>
>>>>>>>>>>> >>>> Should I resize CompressedClassSpaceSize than to
>>>>>>>>>>> show
>>>>>>>>>>> error message?
>>>>>>>>>>> >>> If you add slightly better heuristics for the setup
>>>>>>>>>>> of
>>>>>>>>>>> the
>>>>>>>>>>> CompressedClassSpaceSize flag, for example lowering the
>>>>>>>>>>> CompressedClassSpaceSize when MaxMetaspaceSize is set,
>>>>>>>>>>> then
>>>>>>>>>>> it
>>>>>>>>>>> might be less likely that you'll hit the
>>>>>>>>>>> OutOfMemoryError
>>>>>>>>>>> when
>>>>>>>>>>> the system is set up with strict overcommit settings.
>>>>>>>>>>> >>
>>>>>>>>>>> >> I've uploaded new webrev:
>>>>>>>>>>> >>
>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.01/
>>>>>>>>>>>
>>>>>>>>>>> <http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.01/>
>>>>>>>>>>> >>
>>>>>>>>>>> >> This patch checkes MaxMetaspaceSize,
>>>>>>>>>>> CompressedClassSpaceSize, and
>>>>>>>>>>> >> InitialBootClassLoaderMetaspaceSize.
>>>>>>>>>>> >>
>>>>>>>>>>> >> I add to check CompressedClassSpaceSize in
>>>>>>>>>>> Arguments::set_use_compressed_klass_ptrs().
>>>>>>>>>>> >> If InitialBootClassLoaderMetaspaceSize is greater
>>>>>>>>>>> than
>>>>>>>>>>> MaxMetaspaceSize,
>>>>>>>>>>> >> VM will fail with error message.
>>>>>>>>>>> >>
>>>>>>>>>>> >> InitialBootClassLoaderMetaspaceSize will be set to
>>>>>>>>>>> MaxMetaspaceSize
>>>>>>>>>>> >> when UseCompressedClassPointers is not set in
>>>>>>>>>>> Metaspace::ergo_initialize().
>>>>>>>>>>> >>
>>>>>>>>>>> >>
>>>>>>>>>>> >> Thanks,
>>>>>>>>>>> >>
>>>>>>>>>>> >> Yasumasa
>>>>>>>>>>> >>
>>>>>>>>>>>
>>>>>>>>>>>
>>
More information about the hotspot-runtime-dev
mailing list