[Fwd: Re: RFR (M/L): 8010722 assert: failed: heap size is too big for compressed oops]

[Thomas Schatzl]

Hi all,

  I updated the change for 8010722 per the suggestions of David and
Stefan and Bengt.

Changes:
- add a new OS class hook called init_ergo() to be called between
os::init() and os::init_2().
I preferred this more descriptive name rather than using os::init_2()
and changing all os::init_2() calls to os_init_3().
- sorted new #include references
- naming changes:
All methods using a conservative maximum heap alignment have the name
"conservative" in them now. Non-"conservative" methods return an actual
required alignment.
- test case refactoring:
  - use specific WhiteBox method returning the maximum COOP size
directory instead of parsing some text output
  - use the HotspotDiagnosticMXBean to retrieve the current
ClassMetaspaceSize for the test, i.e. the test case that uses different
class meta space size.

There is need to refactor jtreg support routines across the gc component
for commonly used functionality. I filed JDK-8024157 for this purpose.

The latest version can be viewed at
http://cr.openjdk.java.net/~tschatzl/8010722/webrev.4

bugs.sun.com
http://bugs.sun.com/view_bug.do?bug_id=8010722

JIRA:
https://jbs.oracle.com/bugs/browse/JDK-8010722

Testing:
jprt, jtreg

Thanks,
  Thomas

On Fri, 2013-08-30 at 13:28 +1000, David Holmes wrote:
> Hi Thomas,
> 
> I think it is bad form to call os::init_large_pages() from inside the 
> arguments processing code! I presume you can not move it from 
> os::init_2() to os::init()? I'd prefer to see the argument processing 
> refactored if we have to add in another os::init hook, than to hide this 
> inside the argument processing.
> 
> David
> 
> On 30/08/2013 12:28 AM, Thomas Schatzl wrote:
> > Hi all,
> >
> >    forwarding the RFR for 8010722 here too as it contains changes on
> > parts of the runtime too (argument processing, large page
> > initialization), and you might be interested.
> >
> > Note that the current webrev is
> > http://cr.openjdk.java.net/~tschatzl/8010722/webrev.3
> >
> > Sorry for not thinking about cc'ing here earlier,
> >    Thomas
> >
> > -------- Forwarded Message --------
> >> From: Thomas Schatzl <thomas.schatzl at oracle.com>
> >> To: hotspot-gc-dev at openjdk.java.net
> >> Subject: Re: RFR (M/L): 8010722 assert: failed: heap size is too big
> >> for compressed oops
> >> Date: Wed, 28 Aug 2013 15:32:46 +0200
> >>
> >> Hi all,
> >>
> >> On Tue, 2013-05-14 at 16:37 +0200, Thomas Schatzl wrote:
> >>> Hi all,
> >>>
> >> JIRA:
> >>> https://jbs.oracle.com/bugs/browse/JDK-8010722
> >>
> >>   can I have reviews 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.1/
> >>
> >> There is a new webrev at
> >> http://cr.openjdk.java.net/~tschatzl/8010722/webrev.2 ; this has become
> >> necessary after the changes in CR 8007074 (Stefan K's large pages
> >> changes) and CR 8003424 from Harold.
> >>
> >> Particularly the latter changed how the class metadata space is
> >> allocated, removing a lot of additional checking and alignment issues.
> >>
> >> However the core issue still persists, the maximum heap size for
> >> compressed oops is calculated wrongly as the NULL page is not properly
> >> accounted for.
> >>
> >> testing:
> >> jprt, jprt test cases
> >>
> >> Thomas
> >>
> >>> 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
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >
> >