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

[Jon Masamitsu]

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.

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
>