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

[Vladimir Kozlov]

The change is reasonable.

So why we did not see this problem before?

Thanks,
Vladimir

On 3/8/13 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