RFR(s): 8067187: -XX:MaxMetaspaceSize=20m -Xshare:dump caused JVM to crash
Stefan Karlsson
stefan.karlsson at oracle.com
Tue Jan 13 19:44:04 UTC 2015
At some point during this review the hotspot-runtime-dev list was dropped.
I have one comment that I made to Calvin that I want to add to this thread.
On 07/01/15 15:50, Stefan Karlsson wrote:
> Hi Calvin,
>
> On 2014-12-20 00:14, Calvin Cheung wrote:
>> Hi Stefan,
>>
>> I've an updated webrev at:
>> http://cr.openjdk.java.net/~ccheung/8067187/webrev.02/
>
> Thanks for the updates.
>
> I just realized that the the error message produced by:
> 3342 report_insufficient_metaspace(MetaspaceAux::committed_bytes() +
> word_size * BytesPerWord);
>
> could be misleading. Even if you increasing the MaxMetaspaceSize to
> the given value, the next allocation will most likely fail.
If you set MaxMetaspaceSize to a lower value, say 10m, then you get this
recommendation:
$ build/linux-x86_64-normal-server-fastdebug/images/jdk/bin/java
-XX:MaxMetaspaceSize=10m -Xshare:dump
Java HotSpot(TM) 64-Bit Server VM warning:
The MaxMetaspaceSize of 10485760 bytes is not large enough.
Either don't specify the -XX:MaxMetaspaceSize=<size>
or increase the size to at least 16777216.
but trying with that size doesn't work, and you get a new suggestion:
$ build/linux-x86_64-normal-server-fastdebug/images/jdk/bin/java
-XX:MaxMetaspaceSize=16777216 -Xshare:dump
Java HotSpot(TM) 64-Bit Server VM warning:
The MaxMetaspaceSize of 16777216 bytes is not large enough.
Either don't specify the -XX:MaxMetaspaceSize=<size>
or increase the size to at least 33554432.
This is the problem that I wanted to point out with my previous comment.
StefanK
>
>
> And a nit: report_insufficient_metaspace takes an uintx and you pass
> in a size_t:
>
> 276 void report_insufficient_metaspace(uintx required_size) {
> 277 warning("\nThe MaxMetaspaceSize of " UINTX_FORMAT " bytes is
> not large enough.\n"
> 278 "Either don't specify the -XX:MaxMetaspaceSize=<size>\n"
> 279 "or increase the size to at least " UINTX_FORMAT ".\n",
> 280 MaxMetaspaceSize, required_size);
> 281 exit(2);
> 282 }
>
> Could you change the parameter type to use size_t and the second
> UINTX_FORMAT to SIZE_FORMAT?
>
> Thanks,
> StefanK
>
>>
>> thanks,
>> Calvin
>>
>> On 12/19/2014 9:26 AM, Calvin Cheung wrote:
>>> Hi Stefan,
>>>
>>> On 12/19/2014 2:44 AM, Stefan Karlsson wrote:
>>>> Hi Calvin,
>>>>
>>>> On 19/12/14 07:46, Calvin Cheung wrote:
>>>>> Hi Stefan,
>>>>>
>>>>> On 12/17/2014 12:59 AM, Stefan Karlsson wrote:
>>>>>> Hi Calvin,
>>>>>>
>>>>>> On 2014-12-12 21:42, Calvin Cheung wrote:
>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8067187
>>>>>>>
>>>>>>> This fix is to add a check on the MaxMetaspaceSize when
>>>>>>> performing CDS archive dumping.
>>>>>>>
>>>>>>> With the fix, instead of a crash, it will print and error
>>>>>>> message like the following and exit:
>>>>>>>
>>>>>>> Java HotSpot(TM) 64-Bit Server VM warning:
>>>>>>> The MaxMetaspaceSize of 20971520 bytes is not large enough.
>>>>>>> Either don't specify the -XX:MaxMetaspaceSize=<size>
>>>>>>> or increase the size to at least 33554432.
>>>>>>>
>>>>>>> Tested manually via command line and jtreg test on the new test.
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8067187/webrev/
>>>>>>
>>>>>> Please, don't add this check and side-effect deep down in
>>>>>> MetaspaceGC::can_expand. I think it would be better if you move
>>>>>> this check further up in the call chain. Maybe
>>>>>> Metaspace::initialize would be a good place. We already do CDS
>>>>>> specific checks in that method, for example:
>>>>>> assert(!DumpSharedSpaces || new_chunk != NULL, "should have
>>>>>> enough space for both chunks");
>>>>>
>>>>> Thanks for your review.
>>>>>
>>>>> I've moved the check to Metaspace::initialize() before calling
>>>>> get_initialization_chunk().
>>>>> Also clarified a comment in Metaspace::global_initialize().
>>>>>
>>>>> updated webrev is at the same location:
>>>>> http://cr.openjdk.java.net/~ccheung/8067187/webrev/
>>>>
>>>> Thanks for moving the check.
>>>>
>>>> Please, don't do that. Just add a new version instead. Otherwise
>>>> it's harder to follow what's has changed between the review mails.
>>> I usually save the previous version to webrev.nn.
>>> In this case, the previous version is webrev.00.
>>>>
>>>> http://cr.openjdk.java.net/~ccheung/8067187/webrev/src/share/vm/utilities/debug.cpp.frames.html
>>>>
>>>>
>>>> 276 void report_insufficient_metaspace(uintx required_size) {
>>>> 277 warning("\nThe MaxMetaspaceSize of %d bytes is not large
>>>> enough.\n"
>>>> 278 "Either don't specify the
>>>> -XX:MaxMetaspaceSize=<size>\n"
>>>> 279 "or increase the size to at least %d.\n",
>>>> 280 MaxMetaspaceSize, required_size);
>>>> 281 exit(2);
>>>> 282 }
>>>> 283
>>>>
>>>> You need to use UINTX_FORMAT and not %d to print required_size and
>>>> MaxMetaspaceSize.
>>> I'll fix it.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~ccheung/8067187/webrev/src/share/vm/memory/metaspace.cpp.frames.html
>>>>
>>>>
>>>> I think this would work:
>>>>
>>>> 3336 // for dumping shared archive, make sure the
>>>> MaxMetaspaceSize is large enough.
>>>> 3337 if (DumpSharedSpaces) {
>>>> 3338 size_t metaspace_size = MetaspaceAux::committed_bytes() +
>>>> word_size * BytesPerWord;
>>>> 3339 if (metaspace_size > MaxMetaspaceSize) {
>>>> 3340 report_insufficient_metaspace(metaspace_size);
>>>> 3341 }
>>>> 3342 }
>>>>
>>>>
>>>> but wouldn't it be better to check the results from the actual
>>>> allocations?:
>>>>
>>>> 3344 Metachunk* new_chunk = get_initialization_chunk(NonClassType,
>>>> 3345 word_size,
>>>> 3346 vsm()->medium_chunk_bunch());
>>>> 3347 assert(!DumpSharedSpaces || new_chunk != NULL, "should have
>>>> enough space for both chunks");
>>>> 3348 if (new_chunk != NULL) {
>>>> 3349 // Add to this manager's list of chunks in use and
>>>> current_chunk().
>>>> 3350 vsm()->add_chunk(new_chunk, true);
>>>> 3351 }
>>>
>>> I'll place the check before the assert statement as follows:
>>> // For dumping shared archive, report error if allocation has failed.
>>> if (DumpSharedSpaces && new_chunk == NULL) {
>>> report_insufficient_metaspace(MetaspaceAux::committed_bytes() +
>>> word_size * BytesPerWord);
>>> }
>>>
>>>>
>>>> and:
>>>> 3354 if (using_class_space()) {
>>>> 3355 Metachunk* class_chunk = get_initialization_chunk(ClassType,
>>>> 3356 class_word_size,
>>>> 3357 class_vsm()->medium_chunk_bunch());
>>>> and report the error if that fails?
>>>
>>> I'll do the following (text in green is new code):
>>>
>>> // Allocate chunk for class metadata objects
>>> if (using_class_space()) {
>>> Metachunk* class_chunk = get_initialization_chunk(ClassType,
>>> class_word_size,
>>> class_vsm()->medium_chunk_bunch());
>>> if (class_chunk != NULL) {
>>> class_vsm()->add_chunk(class_chunk, true);
>>> } else {
>>> // For dumping shared archive, report error if allocation has
>>> failed.
>>> if (DumpSharedSpaces) {
>>> report_insufficient_metaspace(MetaspaceAux::committed_bytes() +
>>> class_word_size * BytesPerWord);
>>> }
>>> }
>>> }
>>>>
>>>>
>>>> Could you also capitalize this comment correctly:
>>>> 3336 // for dumping shared archive, make sure the
>>>> MaxMetaspaceSize is large enough.
>>> This will be fixed.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~ccheung/8067187/webrev/test/runtime/SharedArchiveFile/MaxMetaspaceSize.java.html
>>>>
>>>>
>>>> I think you should mention CDS in this summary:
>>>> 27 * @summary Testing the -XX:MaxMetaspaceSize=<size> option
>>> Sure.
>>>
>>> thanks,
>>> Calvin
>>>>
>>>> Thanks,
>>>> StefanK
>>>>
>>>>> I've re-ran the test via jprt.
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>>
>>>>>> Thanks,
>>>>>> StefanK
>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list