RFR: JDK-8087291: InitialBootClassLoaderMetaspaceSize and CompressedClassSpaceSize should be checked consistent from MaxMetaspaceSize
David Holmes
david.holmes at oracle.com
Thu Oct 12 04:55:52 UTC 2017
On 12/10/2017 2:47 PM, Yasumasa Suenaga wrote:
> Hi David,
>
>> You may have time to fix these in place before anyone else sees the webrev
>
> I've fixed that:
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.07/
>
>
> I checked raw text of your email, so I believe the indent is correct.
> If I have mistake(s), please tell me.
Sorry they are off. Basic rules:
var =
some long value;
indent by 4.
foo(arg1, arg2,
arg3, arg4);
align the args on the two lines as shown.
Hope that is clearer.
Thanks,
David
>
> Thanks,
>
> Yasumasa
>
>
> 2017-10-12 13:22 GMT+09:00 David Holmes <david.holmes at oracle.com>:
>> 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