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

[Thomas Schatzl]

Hi all,


On Tue, 2013-06-04 at 18:59 -0700, Coleen Phillimore wrote:
> I found it:
> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.1/ 
> <http://cr.openjdk.java.net/%7Etschatzl/8010722/webrev.1/>
> 
> I looked at arguments.cpp and universe.cpp for the runtime. Looks good.

  sorry for that - and another thanks for looking at runtime changes.

Thanks,
  Thomas

> On 6/4/2013 11:17 AM, Thomas Schatzl wrote:
> > Hi all,
> >
> >    please have a look at a slightly updated version of this CR after
> > comments from SQE and runtime.
> >
> > Changes:
> > - moved the comment that explained the choice of a 100M default
> > ClassMetaSpaceSize to the declaration of this value
> > - changed the default size of ClassMetaSpaceSize to 128M as this is a
> > value that results in less automatic resizing due to alignment (on
> > request).
> > Runtime intends to change this code in the future so that the
> > ClassMetaSpaceSize is not dependent on java heap alignment any more (as
> > far as I understood).
> > - resolved a small merge conflict when updating to latest hotspot-gc:
> > ParallelScavengeHeap::intra_heap_alignment() needs to be a static method
> > to be used in a static context. This is not an issue here as it returns
> > a constant value anyway.
> > - added -XX:+UseParallelGC -XX:-UseParallelOldGC as gc combination to
> > test in the test case. This required some changes in passing down these
> > arguments to the tests themselves.
> >
> > Testing:
> > jtreg tests, jprt
> >
> > There is a small introduction on the change below.
> >
> > On Tue, 2013-05-14 at 16:37 +0200, 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
> >
>