RFR: JDK-8087291: InitialBootClassLoaderMetaspaceSize and CompressedClassSpaceSize should be checked consistent from MaxMetaspaceSize
Yasumasa Suenaga
yasuenag at gmail.com
Thu Oct 12 04:47:20 UTC 2017
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.
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