RFR(s): 8067187: -XX:MaxMetaspaceSize=20m -Xshare:dump caused JVM to crash
Calvin Cheung
calvin.cheung at oracle.com
Fri Dec 19 17:26:38 UTC 2014
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