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

[Bengt Rutisson]

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/

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.

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