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

[Jon Masamitsu]

On 03/11/13 07:34, Bengt Rutisson wrote:
>
> Hi Jon,
>
> On 3/8/13 8:05 PM, Jon Masamitsu wrote:
>> Bengt,
>>
>> With your change the order is
>>
>> set_heap_size()
>> set_ergonomics_flags()
>>
>> set_ergonomics_flags() can turn on UseCompressedOops.
>> set_heap_size() uses UseCompressedOops
>>
>> Right?
>>
>> It might not be a problem today but it makes me nervous.
>>
>> Is there a real circularity there?  I don't know but if there is
>> a way to exclude a circularity, then I won't have to think
>> about it.
>
> Thanks for spotting this, Jon! There is definitely a circular 
> dependency between set_heap_size() and set_ergonomics_flags().
>
> I could not figure out a nice and clean way of breaking the circular 
> dependency. But the circular dependency actually originates from the 
> fact that set_heap_size() does the check that the original bug reports 
> as an issue. It reduces the max heap size if UseCompressedOops is 
> enabled. The problem is that _after_ that it adjust the max heap size 
> to be at least as large as the initial heap size. This is why we crash.
>
> So, one way to fix this is to use the knowledge that the initial heap 
> size is the only thing in set_heap_size() that can increase the max 
> heap size beyond what UseCompressedOops will work with. Here is a 
> webrev for what that could look like:
>
> http://cr.openjdk.java.net/~brutisso/8001049/webrev.01/

Did you intend to call set_use_compressed_oops() before
set_heap_size() and set_ergonomics_flags()?  In the webrev
the call to set_use_compressed_oops() is in
set_ergonmonics_flags().

>
> I think there is another circular dependency already present in the 
> code. If you look at max_heap_for_compressed_oops() its implementation 
> uses ClassMetaspaceSize. But this value may be updated a bit later in 
> set_ergonomics_flags():
>
>     FLAG_SET_ERGO(uintx, ClassMetaspaceSize, 100*M);
>
> Is this a problem? It is not really related to my current change, but 
> if it is a problem we should probably file a bug for it.

I'll file a bug for this.  Thanks.

Jon

>
> Thanks again for looking at this!
> Bengt
>
>>
>>
>> Jon
>>
>>
>> On 3/8/2013 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
>>>
>>
>