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

[Bengt Rutisson]

Vladimir, Jon, Harold and Christian,

Thanks for the reviews!

All set to push now.

Bengt


On 3/12/13 7:35 PM, Vladimir Kozlov wrote:
> 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
>>>>>>>
>>>>>
>>>
>>