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

[Calvin Cheung]

Hi Stefan,

I've an updated webrev at:
     http://cr.openjdk.java.net/~ccheung/8067187/webrev.02/

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