<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 2015-04-15 18:58, Per Liden wrote:<br>
</div>
<blockquote
cite="mid:1335097B-4743-476B-9AAF-7DFFA0DBC276@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
Hi Kim,<br class="">
<br class="">
Thanks for looking at this, sorry for the delayed reply.<br
class="">
<div><br class="">
<blockquote type="cite" class="">
<div class="">On 13 Apr 2015, at 07:42, Kim Barrett <<a
moz-do-not-send="true"
href="mailto:kim.barrett@oracle.com" class="">kim.barrett@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">On Apr 10, 2015, at 10:57 AM, Per Liden <<a
moz-do-not-send="true" href="mailto:per.liden@oracle.com"
class="">per.liden@oracle.com</a>> wrote:<br class="">
<blockquote type="cite" class=""><br class="">
Hi,<br class="">
<br class="">
Universe::initialize_heap() is kind of messy as it
contains lots of #if INCLUDE_ALL_GCS, this patch tries to
make that code a bit more readable. Also, all collectors
except ParallelScavenge take their CollectorPolicy as an
argument to the constructor. This patch aligns
ParallelScavenge to this pattern to further make the code
in initialize_heap() more readable.<br class="">
<br class="">
Bug: <a moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8077417"
class="">https://bugs.openjdk.java.net/browse/JDK-8077417</a><br
class="">
<br class="">
Webrev: <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Epliden/8077417/webrev.0/"
class="">http://cr.openjdk.java.net/~pliden/8077417/webrev.0/</a><br
class="">
</blockquote>
<br class="">
------------------------------------------------------------------------------<br
class="">
src/share/vm/memory/universe.cpp<br class="">
708 jint status;<br class="">
709 <br class="">
710 #if !INCLUDE_ALL_GCS<br class="">
711 if (UseParallelGC) {<br class="">
712 fatal("UseParallelGC not supported in this VM.");<br
class="">
...<br class="">
731 if (status != JNI_OK) {<br class="">
732 return status;<br class="">
<br class="">
I think this might produce a compiler warning for
uninitialized<br class="">
"status" at line 731 for at least some platforms. I think
fatal() is<br class="">
not marked as non-returning, so to the compiler it appears
that<br class="">
control flow can reach line 731 with an uninitialized
"status”.<br class="">
</div>
</blockquote>
<div><br class="">
</div>
<div>I agree, I'll initialize it to JNI_ERR. It seems we don't
get warnings about this today because we don't enable
-Wuninitialized. Stefan K has a patch in the pipe to mark
fatal() as not returning, but until then I'll initialize it.<br
class="">
</div>
<br class="">
<blockquote type="cite" class="">
<div class=""><br class="">
When do we build with INCLUDE_ALL_GCS being false? I'm
guessing only<br class="">
for embedded, and maybe client builds? Were either of those
part of<br class="">
the testing for these changes?<br class="">
</div>
</blockquote>
<div><br class="">
</div>
<div>It's false when we build the VM with
--with-jvm-variants=minimal1, which is used by some embedded
targets (but not client).<br class="">
</div>
<br class="">
<blockquote type="cite" class="">
<div class=""><br class="">
The simplest fix is to initialize status to something,
probably *not*<br class="">
JNI_OK; maybe JNI_ERR?<br class="">
<br class="">
------------------------------------------------------------------------------
<br class="">
src/share/vm/memory/universe.cpp <br class="">
727 else { // UseSerialGC<br class="">
<br class="">
Add a guarantee that UseSerialGC is true here?<br class="">
</div>
</blockquote>
<div><br class="">
</div>
<div>It turns out that we have cases were no gc it selected, and
then we fall back to using serial. There's a bug to fix that:<br
class="">
<br class="">
JDK-8068582: UseSerialGC not always set up properly<br
class="">
<br class="">
I'll add a comment in the code to say that we can't assert
there. Will also add a comment to that bug to add an
assert/guarantee when that bug is fixed.<br class="">
</div>
<br class="">
<blockquote type="cite" class="">
<div class=""><br class="">
------------------------------------------------------------------------------<br
class="">
src/share/vm/memory/universe.cpp <br class="">
735
ThreadLocalAllocBuffer::set_max_size(Universe::heap()->max_tlab_size());<br
class="">
<br class="">
This used to be done before
Universe::heap()->initialize(). Now it is<br class="">
being called after the heap initialization. Is that change
of order<br class="">
ok?<br class="">
<br class="">
I looked at it and it seems ok, in which case I think the
change of<br class="">
order is actually an improvement.<br class="">
</div>
</blockquote>
<div><br class="">
</div>
<div>I looked at when I did the patch and concluded that it was
ok. In fact, it seemed a bit strange to set that up before
calling initialize().<br class="">
</div>
<br class="">
<blockquote type="cite" class="">
<div class=""><br class="">
While I was looking around I noticed what might be a
separate problem.<br class="">
It seems possible with a sufficiently large heap that G1's<br
class="">
max_tlab_size computation might exceed the
int[Integer.MAX_VALUE] size<br class="">
limit discussed in CollectedHeap::max_tlab_size. I'm not
sure whether<br class="">
that limit is actually relevant to G1 though. If it is
relevant then<br class="">
a new CR should be filed.<br class="">
</div>
</blockquote>
<div><br class="">
</div>
</div>
I don't think there's a problem there, G1's max_tlab_size will
return (region_size_in_words / 2) - 1 words, and G1's region size
at most 32M at the moment. Should be way below JINT_MAX.<br
class="">
<br class="">
<br class="">
Here's an updated webrev:<br class="">
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Epliden/8077417/webrev.1/"
class="">http://cr.openjdk.java.net/~pliden/8077417/webrev.1/</a><br
class="">
<br class="">
Diff against first webrev:<br class="">
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Epliden/8077417/webrev.1-diff_0vs1/"
class="">http://cr.openjdk.java.net/~pliden/8077417/webrev.1-diff_0vs1/</a><br
class="">
</blockquote>
<br>
Looks good.<br>
<br>
StefanK<br>
<br>
<blockquote
cite="mid:1335097B-4743-476B-9AAF-7DFFA0DBC276@oracle.com"
type="cite"><br class="">
cheers,<br class="">
/Per
<div class=""><br class="">
</div>
</blockquote>
<br>
</body>
</html>