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

[Jon Masamitsu]

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: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130312/da2886b1/attachment.html