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

[Bengt Rutisson]

Hi Christian,

Thanks for verifying this change!

I agree that the results look like what we want.

Thanks again for doing the extra testing!
Bengt

On 3/12/13 1:11 PM, Christian Törnqvist wrote:
> 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