RFR (M/L): 8010722 assert: failed: heap size is too big for compressed oops (possibly CR 8009778)

[Thomas Schatzl]

Hi,

On Mon, 2013-05-20 at 12:19 -0700, Jon Masamitsu wrote:
> Thomas,
> 
> http://cr.openjdk.java.net/~tschatzl/8010722/webrev/src/share/vm/runtime/arguments.cpp.udiff.html
> 
> > +  if (apply_ergonomics_on_classmetaspacesize()) {
> > +    max_class_metaspace_size = MAX2(max_class_metaspace_size, ErgoClassMetaspaceSize);
> > +  }
>
> Why does this if-test work?  I think the ErgoClassMetaspaceSize is
> used when UseCompressedKlassPointers is true.

I am not sure what the question is because I cannot see a problem with
the code. Could you please point out what is wrong here?

When looking at these changes, consider that at the point where we try
to determine the maximum heap size for compressed oops we do not know
whether or not UseCompressedOops (and UseCompressedKlassPointers) is
used later. Both values may be set ergonomically later. Depending on
their value max_heap_for_compressed_oops() would be different...

So this change goes the extreme
Maybe you have a better idea to handle this situation, not needing to be
that conservative?ly conservative route and takes into account all
possible ergonomics decisions as it is not easily possible to later
adapt heap/generation sizes again (which might change the ergonomic
decisions, which might affect heap/generation sizes, ...).

ErgoClassMetaspaceSize in particular is used if
UseCompressedKlassPointers (which is depends on a possibly ergonomically
determined UseCompressedOops which we are basically determining
here...), and the apply_ergonomics_on_classmetaspacesize() predicate is
true (i.e. ClassMetaspaceSize has a default value).

I.e. the predicate for this MAX2() operation and the ergonomic decision
in arguments.cpp:1504 (in the new code; old: arguments.cpp:1461) must be
the same that we catch this case - so I extracted them into a method.
Also the ergonomically determined class metaspace size value, which has
been factored out into the ErgoClassMetaspaceSize constant.
> 
Btw, while looking at the default value for ErgoClassMetaspaceSize
default value, is there a reason to set it to 100M? (This value has been
hardcoded earlier, see arguments.cpp:1467 in the original code). 100M
seems to be an unusual value for the VM which prefers power of 2 or page
size aligned values.

> Why is the class metaspace size being aligned up by the max heap
> alignment?

This is due to Universe::reserve_heap() doing the same. This is
apparently because the heap itself must be aligned to that alignment,
but the class metaspace size is located before the heap, so in effect
the VM has to align class metaspace size to the same alignment before
reserving memory. (Which is why I think 100M is an unusual value; I'd
have expected something like 128M or so).

See the comment in Universe::reserve_heap().

> 1380   size_t aligned_metaspace_size = align_size_up_(max_class_metaspace_size, max_heap_alignment());
> 
> Jon

Maybe you have a better idea to handle this situation, not needing to be
that conservative?

Thanks for looking at this,
  Thomas

> 
> On 5/14/13 7:37 AM, Thomas Schatzl wrote:
> 
> > Hi all,
> > 
> >   can I have a review for the following change?
> > 
> > In argument processing related to ergonomically determining compressed
> > oops use, there were a few use-before-set issues leading to crashes that
> > this fix tries to resolve.
> > 
> > bugs.sun.com
> > http://bugs.sun.com/view_bug.do?bug_id=8010722
> > 
> > JIRA:
> > https://jbs.oracle.com/bugs/browse/JDK-8010722
> > 
> > webrev:
> > http://cr.openjdk.java.net/~tschatzl/8010722/webrev/
> > 
> > testing:
> > jprt, test cases
> > 
> > Here's a walkthrough of the changes:
> > 
> > As mentioned, the cause of this CR is that ergonomics for determining
> > the maximum java heap size usable for compressed oops uses variables
> > that are later changed ergonomically.
> > 
> > It is best to look at the changes beginning from
> > Arguments::set_ergonomics_flags(): the idea of this change is to avoid
> > later overflow, so the change tries to conservatively estimate sizes of
> > the non-java heap parts. The complication is that not even the later
> > effective alignment of these heap parts has been determined at this
> > point.
> > 
> > So the change first calculates the maximum possible heap alignment by
> > calling set_max_heap_alignment(); this size is influenced by OS page
> > sizes, so the change needs to initialize large pages by calling
> > os::large_page_init() in Arguments::parse(), before the call to
> > set_ergonomics_flags(). The maximum possible alignment is then
> > calculated by asking the active GC for its maximum alignment, as at this
> > point the GC has already been determined, the maximum page size, and
> > other requirements, like alignment for card table size etc.
> > 
> > Now the code can calculate the conservative estimate for actual maximum
> > heap for compressed oops used in set_use_compressed_oops(), by
> > subtracting the conservatively aligned sizes of the other heap parts.
> > (In Arguments::max_heap_for_compressed_oops()) The result is the maximum
> > possible heap that can use compressed oops, minus the aligned metaspace
> > size, minus the aligned null page size.
> > 
> > There is another circular dependency problem here, the metaspace size itself is later ergonomically sized; the change fixes this problem by anticipating that in Arguments::max_heap_for_compressed_oops(), using the same predicate for determining whether to ergonomically size it or not [this is CR8009778 I think].
> > 
> > The other changes are straightforward: the os_* changes result from that
> > large page initialization must be done earlier now; the changes in the
> > collectors themselves are simply about providing the collector's maximum
> > alignment. The change in Universe::reserve_heap() contains one assertion that checks whether the conservative estimate for alignment has been conservative enough earlier.
> > 
> > The test case tests test cases from the CR that work now, and additional
> > border cases related to ergonomically deciding heap size for compressed
> > oops.
> > 
> > One side effect of this change is that the ergonomically determined heap size is slighly smaller now (so that it always fits :).
> > 
> > Thanks,
> >   Thomas
> > 
> > 
> > 
> > 
> > 
>