<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<body bgcolor="#FFFFFF" text="#000000">
Thanks for fixing this! Overall it looks good.<br>
Some minor comments:<br>
The comment on line 155 in the new version of
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
collectorPolicy.cpp should probably start with a capital T:<br>
155 // there <br>
The format for the err_msg should use SIZE_FORMAT instead of %zd.<br>
It would be nice if the #ifdefs in
Arguments::set_largest_max_heap_alignment() could be cleaned up
somehow. I don't have a really good suggestion, just thought it
would be good to try to see if it can be improved somehow.<br>
The test case seems to be missing. Forgot to hg add it?<br>
Is the move of the calls to os::large_page_init() safe? It has
some side effects.<br>
On 8/28/13 3:32 PM, Thomas Schatzl wrote:<br>
<blockquote cite="mid:1377696766.3661.24.camel@cirrus" type="cite">
<pre wrap="">Hi all,
On Tue, 2013-05-14 at 16:37 +0200, Thomas Schatzl wrote:
<pre wrap="">Hi all,
<pre wrap=""><a class="moz-txt-link-freetext" href="https://jbs.oracle.com/bugs/browse/JDK-8010722">https://jbs.oracle.com/bugs/browse/JDK-8010722</a>
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.
<a class="moz-txt-link-freetext" href="http://bugs.sun.com/view_bug.do?bug_id=8010722">http://bugs.sun.com/view_bug.do?bug_id=8010722</a>
<a class="moz-txt-link-freetext" href="https://jbs.oracle.com/bugs/browse/JDK-8010722">https://jbs.oracle.com/bugs/browse/JDK-8010722</a>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8010722/webrev.1/">http://cr.openjdk.java.net/~tschatzl/8010722/webrev.1/</a>
There is a new webrev at
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8010722/webrev.2">http://cr.openjdk.java.net/~tschatzl/8010722/webrev.2</a> ; 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
jprt, jprt test cases
<pre wrap="">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
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
One side effect of this change is that the ergonomically determined heap size is slighly smaller now (so that it always fits :).