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

[Bengt Rutisson]

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