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

[Stefan Karlsson]

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.

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.


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   }

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?


Could you also capitalize this comment correctly:
3336   // for dumping shared archive, make sure the MaxMetaspaceSize is large enough.


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


Thanks,
StefanK

> I've re-ran the test via jprt.
>
> thanks,
> Calvin
>>
>> Thanks,
>> StefanK
>>
>>>
>>> thanks,
>>> Calvin
>>
>