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

[Bengt Rutisson]

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