<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Am 12.07.2017 um 14:20 schrieb Stefan
Johansson:<br>
</div>
<blockquote type="cite"
cite="mid:7701ab0e-7a75-baec-434f-62978a90e8e9@oracle.com">Hi
Roman,
<br>
<br>
On 2017-07-12 12:47, Roman Kennke wrote:
<br>
<blockquote type="cite">Am 12.07.2017 um 08:44 schrieb Per Liden:
<br>
<blockquote type="cite">Hi Roman,
<br>
<br>
On 2017-07-11 16:17, Stefan Johansson wrote:
<br>
<blockquote type="cite">Hi Roman,
<br>
<br>
On 2017-07-11 08:34, Per Liden wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
On 2017-07-10 18:35, Roman Kennke wrote:
<br>
<blockquote type="cite">Hi Per,
<br>
<br>
thanks for the review!
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">AdaptiveSizePolicy is not
used/called from outside the GCs, and not
<br>
all
<br>
GCs need them. It makes sense to remove it from the
CollectedHeap
<br>
and
<br>
CollectorPolicy interfaces and move them down to the
actual
<br>
subclasses
<br>
that used them.
<br>
<br>
I moved AdaptiveSizePolicyOutput to
parallelScavengeHeap.hpp, it's
<br>
only
<br>
used/implemented in the parallel GC. Also, I made
this class
<br>
AllStatic
<br>
(was StackObj)
<br>
</blockquote>
AdaptiveSizePolicyOutput::print() is actually called
from
<br>
runtime/java.cpp also, so it's used outside of
ParallelGC. I'm fine
<br>
with moving it, but we should have the proper
#includes in java.cpp.
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
I just realized that this doesn't build on linux-i586, which
builds a
<br>
minimal JVM where INCLUDE_ALL_GCS isn't defined (and will thus
not
<br>
include parallelScavangeHeap.hpp). Rather than having some
ugly #ifdef
<br>
INCLUDE_ALL_GCS at the AdaptiveSizePolicyOutput::print() call
site I
<br>
suggest we keep AdaptiveSizePolicyOutput in
adaptiveSizePolicy.hpp for
<br>
now.
<br>
</blockquote>
I tried that. Unfortunately, it also requires #ifdef
INCLUDE_ALL_GCS to
<br>
be able to call ParallelScavengeHeap::heap(), or else defeats
the
<br>
purpose of this patch by requiring CollectedHeap to still carry
<br>
size_policy().. which we don't want. In addition to that, if I
try to
<br>
include parallelScavengeHeap.hpp in adaptiveSizePolicy.hpp, I am
getting
<br>
freaky circular dependency problems. #ifdef INCLUDE_ALL_GCS in
java.cpp
<br>
around AdaptiveSizePolicyOutput seems like the lesser evil...
done so here:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~rkennke/8179268/webrev.04/">http://cr.openjdk.java.net/~rkennke/8179268/webrev.04/</a>
<br>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Erkennke/8179268/webrev.04/"><http://cr.openjdk.java.net/%7Erkennke/8179268/webrev.04/></a>
<br>
<br>
Ok?
<br>
</blockquote>
I'm no big fan of having #if INCLUDE_ALL_GCS if it can be avoided.
A few lines below the call to AdaptiveSizePolicyOutput::print(),
we call Universe::heap()->print_tracing_info(). I think we
could move AdaptiveSizePolicyOutput::print() into
ParallelScavengeHeap::print_tracing_info() without running into
any problems.
<br>
<br>
What do you think about that solution?
<br>
</blockquote>
<br>
That's a very good idea!! It alters behaviour slightly (will print
adaptive size policy stuff in hs_err now) but I think that's for the
better.<br>
<br>
Incremental:<br>
<a
href="http://cr.openjdk.java.net/%7Erkennke/8179268/webrev.05.diff/">http://cr.openjdk.java.net/~rkennke/8179268/webrev.05.diff/</a><br>
Full:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Erkennke/8179268/webrev.05">http://cr.openjdk.java.net/~rkennke/8179268/webrev.05</a><br>
<br>
Good now?<br>
<br>
Thanks,<br>
Roman<br>
<br>
</body>
</html>