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

[Vladimir Kozlov]

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.

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.

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