RFR: JDK-8087291: InitialBootClassLoaderMetaspaceSize and CompressedClassSpaceSize should be checked consistent from MaxMetaspaceSize
Yasumasa Suenaga
yasuenag at gmail.com
Thu Oct 12 05:16:01 UTC 2017
Thanks David!
I've fixed:
http://cr.openjdk.java.net/~ysuenaga/JDK-8087291/webrev.08/
Yasumasa
2017-10-12 13:55 GMT+09:00 David Holmes <david.holmes at oracle.com>:
> 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