Request for review (S): 8001049: VM crashes when running with large -Xms and not specifying ObjectAlignmentInBytes

Tue Mar 12 12:11:24 UTC 2013

Hi Bengt,

Did some sanity testing using one of your builds vs jdk8-b79 on a Linux x64 machine (sthoel01.se.oracle.com which has 144GB of memory):

-Xms50g -Xmx60g No difference, no coops
-Xms50g -Xmx60g -XX:+UseCompressedOops, No difference, no coops, warning printed: "Max heap size too large for Compressed Oops"
-Xms50g -Xmx60g -XX:+UseCompressedOops -XX:ObjectAlignmentInBytes=16, No difference, coops enabled
-Xms50g -Xmx60g -XX:ObjectAlignmentInBytes=16, No difference, coops enabled

-Xms50g jdk8-b79 crashes, fixed jvm starts without coops and no warning messages
-Xms50g -XX:+UseCompressedOops jdk8-b79 crashes, fixed jvm prints warning: "Max heap size too large for Compressed Oops"
-Xms50g -XX:+UseCompressedOops -XX:ObjectAlignmentInBytes=16, No difference, coops enabled
-Xms50g -XX:ObjectAlignmentInBytes=16, No difference, coops enabled

Code change looks good and behavior seems consistent with earlier versions.

Thanks,
Christian

-----Original Message-----
From: Bengt Rutisson 
Sent: den 12 mars 2013 08:41
To: Vladimir Kozlov
Cc: hotspot-gc-dev openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
Subject: Re: Request for review (S): 8001049: VM crashes when running with large -Xms and not specifying ObjectAlignmentInBytes

Hi Vladimir,

On 3/11/13 7:36 PM, Vladimir Kozlov wrote:
> On 3/11/13 7:57 AM, Bengt Rutisson wrote:
>>
>> Hi Vladimir,
>>
>> Thanks for looking at this!
>>
>> On 3/8/13 7:09 PM, Vladimir Kozlov wrote:
>>> I am wondering should we also add a check into
>>> Universe::reserve_heap() before calling 
>>> Universe::preferred_heap_base().
>>
>> Do you mean that we should add an assert in Universe::reserve_heap() 
>> to check that the size we reserve is compatible with the compressed 
>> oop
>
> Yes, I want
>
>    size_t total_reserved = align_size_up(heap_size + 
> Universe::class_metaspace_size(), alignment);
> +  assert(!UseCompressedOops || (total_reserved <= (OopEncodingHeapMax
> - os::vm_page_size())), "heap size is too big for compressed oops");
>    char* addr = Universe::preferred_heap_base(total_reserved,
> Universe::UnscaledNarrowOop);
>
> You don't need to duplicate max_heap_for_compressed_oops() because 
> total_reserved includes class_metaspace.

Thanks for this clarification! I added the assert.

> There are size rounding codes in some places after arguments parsing 
> and someone could add an other ergonomic code after checks during 
> arguments parsing. To have an assert to catch such cases would be nice.

Good point. Thanks for the help with this.

Bengt

>
> Thanks,
> Vladimir
>
>> state? I think that's fine but that means that I either have to 
>> duplicate the code in max_heap_for_compressed_oops() or make it 
>> publicly available somewhere. Currently it is just a local function 
>> inside Arguments.cpp.
>>
>> Personally I am not sure the assert will be worth the code change, 
>> but I'm fine with adding it if you want to.
>>
>> Thanks,
>> Bengt
>>
>>
>>>
>>> Vladimir
>>>
>>> On 3/8/13 9:41 AM, harold seigel wrote:
>>>> The change looks good.  Perhaps this problem wasn't seen before 
>>>> because this scenario hadn't been tested?
>>>>
>>>> Harold
>>>>
>>>> On 3/8/2013 11:17 AM, Vladimir Kozlov wrote:
>>>>> The change is reasonable.
>>>>>
>>>>> So why we did not see this problem before?
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 3/8/13 5:20 AM, Bengt Rutisson wrote:
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> Could I have a couple of reviews for this change please?
>>>>>> http://cr.openjdk.java.net/~brutisso/8001049/webrev.00/
>>>>>>
>>>>>> I'm sending this to both the GC and the runtime teams since I 
>>>>>> think compressed oops is mixed responsibility for both teams.
>>>>>>
>>>>>> Background (mostly from the bug report):
>>>>>>
>>>>>> Hotspot crashes if it is run with a large initial size:
>>>>>>
>>>>>>  >./java -Xms70g -version
>>>>>> #
>>>>>> # A fatal error has been detected by the Java Runtime Environment:
>>>>>> #
>>>>>> # SIGSEGV (0xb) at pc=0x0000000000000000, pid=14132,
>>>>>> tid=140305232803584
>>>>>>
>>>>>> The reason is that we enable UseCompressedOops but we use the 
>>>>>> default value for ObjectAlignmentInBytes. With a large heap size 
>>>>>> we would have needed to adjust the object alignment to be able to 
>>>>>> use compressed oops.
>>>>>>
>>>>>> However, after reviewing the code it looks like the fix is not to 
>>>>>> try to adjust the object alignment but rather to not enable 
>>>>>> compressed oops for large heaps. If someone wants to use 
>>>>>> compressed oops on a very large heap they need to explicitly set 
>>>>>> both UseCompressedOops and ObjectAlignmentInBytes on the command 
>>>>>> line. As far as I can tell this is how it is intended to work.
>>>>>>
>>>>>> Here is the reason for the crash and the rational behind the fix:
>>>>>>
>>>>>> In Arguments::set_ergonomics_flags() we check that the max heap 
>>>>>> size is small enough before we enable compressed oops:
>>>>>>
>>>>>>    if (MaxHeapSize <= max_heap_for_compressed_oops()) { #if 
>>>>>> !defined(COMPILER1) || defined(TIERED)
>>>>>>      if (FLAG_IS_DEFAULT(UseCompressedOops)) {
>>>>>>        FLAG_SET_ERGO(bool, UseCompressedOops, true);
>>>>>>      }
>>>>>> #endif
>>>>>>
>>>>>> And after that we print a warning message if the heap is too large:
>>>>>>
>>>>>>      if (UseCompressedOops && !FLAG_IS_DEFAULT(UseCompressedOops)) {
>>>>>>        warning("Max heap size too large for Compressed Oops");
>>>>>>        FLAG_SET_DEFAULT(UseCompressedOops, false);
>>>>>>        FLAG_SET_DEFAULT(UseCompressedKlassPointers, false);
>>>>>>      }
>>>>>>
>>>>>> Now the problem is that when we don't set the max heap size on 
>>>>>> the command line it will be adjusted based on the initial size 
>>>>>> (-Xms) and this happens in Arguments::set_heap_size(), which is 
>>>>>> called *after*
>>>>>> Arguments::set_ergonomics_flags() has been called. So, when we do 
>>>>>> the check against the max size in 
>>>>>> Arguments::set_ergonomics_flags(), we check against the default 
>>>>>> value for the max size. This value fits well with a compressed 
>>>>>> heap, so we enable compressed oops and crash later on when we 
>>>>>> can't address the upper part of the heap.
>>>>>>
>>>>>> The fix is to move the call to set_heap_size() to *before* the 
>>>>>> call to set_ergonomics_flags(). This way the check is done 
>>>>>> against the correct value. This has two effects:
>>>>>>
>>>>>> 1) We don't enable UseCompressedOops on heap sizes that are too 
>>>>>> large
>>>>>> 2) If someone sets -XX:+UseCompressedOops on the command line but 
>>>>>> specifies a too large heap a warning message will be logged and 
>>>>>> UseCompressedOops will be turned off.
>>>>>>
>>>>>> I am always hesitant to rearrange the order of calls in 
>>>>>> Arguments::parse(). But in this case it is necessary to get the 
>>>>>> correct behavior and I think it is safe. As far as I can tell 
>>>>>> there is no other code between the two methods that try to read 
>>>>>> the MaxHeapSize value.
>>>>>>
>>>>>> Thanks,
>>>>>> Bengt
>>>>
>>