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

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir

On 3/12/13 12:48 AM, Bengt Rutisson wrote:
>
> Updated webrev based on the comments from Vladimir, Jon and Harold:
>
> http://cr.openjdk.java.net/~brutisso/8001049/webrev.02/
>
> Thanks,
> Bengt
>
>
> On 3/12/13 8:40 AM, Bengt Rutisson wrote:
>>
>> 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
>>>>>>
>>>>
>>
>