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

[Bengt Rutisson]

Hi Jon,

On 3/11/13 4:45 PM, Jon Masamitsu wrote:
>
>
> 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 left the call to set_use_compressed_oops() inside 
set_ergonomics_flags() on purpose. This was both for minimizing the risk 
of the change (everything is now done in the exact same order as before) 
and for making sure that set_use_compressed_oops() has been called 
before we start reading UseCompressedOops inside set_ergonomics_flags().

I am not completely happy with how this looks. It might be better to 
move all of the compressed handling out of set_ergonomics_flags(), but 
if it is all right with you I would like to avoid that at the moment.

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

Great, thanks!

Bengt


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