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

[Bengt Rutisson]

Thanks for the review, Jon!

And thanks for filing the refactoring bug!

Bengt

On 3/12/13 4:41 PM, Jon Masamitsu wrote:
>
>
> On 03/12/13 00:47, Bengt Rutisson wrote:
>>
>> 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'm OK with the current fix.  Moving the code to deal with
> COOPS seems like a good idea.   I've filed an RFE for this
> issue.
>
> JDK-8009910 - Refactor code in arguments.cpp to break circularity in 
> COOPS settings <https://jbs.oracle.com/bugs/browse/JDK-8009910>
>
> Your fix looks ready to go.
>
> Jon
>>>
>>>>
>>>> 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
>>>>>>
>>>>>
>>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130312/e2b97e62/attachment.htm>