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

[Bengt Rutisson]

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