RFR(s): 8067187: -XX:MaxMetaspaceSize=20m -Xshare:dump caused JVM to crash

Wed Jan 7 14:50:56 UTC 2015

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.

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
>>>>>
>>>>
>>>
>>
>