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

[Calvin Cheung]

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