<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Since it's related to sizing policy fixes, I'd rather push it now if
possible. <br>
<br>
Refactoring CollectorPolicy is another aspect of work, which I would
appreciate someone to do it or I can contribute as well.<br>
<br>
Thanks.<br>
Tao<br>
<br>
On 2/13/2013 6:13 AM, Bengt Rutisson wrote:
<blockquote cite="mid:511B9F7E.2070600@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Tao,<br>
<br>
On 2/12/13 11:24 PM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:511AC129.9080409@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Hi Bengt,<br>
Above all, please confirm with your final say about the change.<br>
</blockquote>
<br>
I think you are correct that this change won't have any effect.
But in that case, is it still worth doing?<br>
<br>
A short while ago, Erik sent out a suggestion for how to refactor
the CollectorPolicy code. Maybe we should just file a bug for
cleaning this up properly? In that bug we could add a comment
about this particular change.<br>
<br>
I'm not sure what the best approach is. I kind of prefer removing
dead code paths rather than trying to fix them. Also, I don't know
how to verify that the fix is correct if it is not being used.<br>
<br>
Anybody else have any opinions on this?<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:511AC129.9080409@oracle.com" type="cite"> <br>
Thanks.<br>
Tao<br>
<br>
On 2/12/2013 12:03 PM, Tao Mao wrote:
<blockquote cite="mid:511AA025.7050603@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Hi all,<br>
<br>
After some investigation of the code, I figured out that the
only opportunity where my change will affect something is in
MarkSweepPolicy, which is supposed to support sizing policy
(if any) for UseSerialGC [1].<br>
<br>
But, I think, SerialGC hasn't implemented any sizing policy
associated with flags MaxGC*PauseMillis (I'm not sure about it
tho/<span class="moz-smiley-s3"></span>). So, we can conclude
that if a customer uses SerialGC, they wouldn't set
MaxGC*PauseMillis anyway. <br>
<br>
(Bengt and Jon, please help verify the above conclusion.)<br>
<br>
Therefore, the change will fix the initialization bug in a
safe way. <br>
<br>
It did take me a while to figure out the class and method
hierarchy. I share the diagram as a pic below. Hopefully, we
can use it to build up a better class design if possible.<br>
<br>
Sorry for the handwriting and not being able to draw a fancier
diagram in text <span class="moz-smiley-s1"></span>.<br>
<br>
Thanks.<br>
Tao<br>
<br>
[1] in Universe::initialize_heap()<br>
<br>
if (<font color="#ff0000">UseParallelGC</font>) {<br>
#ifndef SERIALGC<br>
Universe::_collectedHeap = new ParallelScavengeHeap();<br>
#else // SERIALGC<br>
fatal("UseParallelGC not supported in this VM.");<br>
#endif // SERIALGC<br>
<br>
} else if (<font color="#ff0000">UseG1GC</font>) {<br>
#ifndef SERIALGC<br>
G1CollectorPolicy* g1p = new G1CollectorPolicy();<br>
G1CollectedHeap* g1h = new G1CollectedHeap(g1p);<br>
Universe::_collectedHeap = g1h;<br>
#else // SERIALGC<br>
fatal("UseG1GC not supported in java kernel vm.");<br>
#endif // SERIALGC<br>
<br>
} else {<br>
GenCollectorPolicy *gc_policy;<br>
<br>
if (<font color="#ff0000">UseSerialGC</font>) {<br>
gc_policy = new MarkSweepPolicy();<br>
} else if (<font color="#ff0000">UseConcMarkSweepGC</font>)
{<br>
#ifndef SERIALGC<br>
if (<font color="#ff0000">UseAdaptiveSizePolicy</font>)
{<br>
gc_policy = new ASConcurrentMarkSweepPolicy();<br>
} else {<br>
gc_policy = new ConcurrentMarkSweepPolicy();<br>
}<br>
#else // SERIALGC<br>
fatal("UseConcMarkSweepGC not supported in this VM.");<br>
#endif // SERIALGC<br>
} else { // default old generation<br>
gc_policy = new MarkSweepPolicy();<br>
}<br>
<br>
Universe::_collectedHeap = new
GenCollectedHeap(gc_policy);<br>
}<br>
<br>
<br>
<img src="cid:part1.07030407.03020809@oracle.com" alt=""><br>
<br>
On 2/10/2013 1:34 PM, Bengt Rutisson wrote:
<blockquote cite="mid:51181278.8090401@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
<br>
Hi Tao,<br>
<br>
Thanks for your answers. Some comments inline.<br>
<br>
On 2/9/13 12:44 AM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:51158DCC.2020205@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">I'm trying to answer these
questions. Please see inline. Thanks.<br>
Tao<br>
<br>
On 2/8/13 8:17 AM, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:51152519.9030109@oracle.com"
type="cite"> <br>
Hi Tao, <br>
<br>
I think the change looks good and makes sense. <br>
<br>
But I have a hard time estimating the implications of
this change. <br>
<br>
It seems like MaxGCMinorPauseMillis is only used by CMS
and ParallelScavenge. </blockquote>
You are right. Before the fix, four occurrences below.
(The one in collectorPolicy.cpp is suspected to be wrong)<br>
<br>
MaxGCMinorPauseMillis is introduced in different kinds of
classes for CMS and PS: ConcurrentMarkSweep<b>Policy</b>
and ParalleScavenge<b>Heap</b>.<br>
<br>
$ grep -nr "MaxGCMinorPauseMillis"
src/src/share/vm/gc_implementation/concurrentMarkSweep/cmsCollectorPolicy.cpp:87:
double max_gc_minor_pause_sec = ((double)
MaxGCMinorPauseMillis)/1000.0;<br>
src/share/vm/gc_implementation/parallelScavenge/parallelScavengeHeap.cpp:156:
double max_gc_minor_pause_sec = ((double)
MaxGCMinorPauseMillis)/1000.0;<br>
src/share/vm/memory/collectorPolicy.cpp:170: const double
max_gc_minor_pause_sec = ((double)
MaxGCMinorPauseMillis)/1000.0;<br>
src/share/vm/runtime/globals.hpp:2043: product(uintx,
MaxGCMinorPauseMillis, max_uintx,
\<br>
<blockquote cite="mid:51152519.9030109@oracle.com"
type="cite">For both of those the default for
MaxGCMinorPauseMillis is the same as MaxGCPauseMillis so
it should be fine. <br>
</blockquote>
How did you infer that the default value for
MaxGCMinorPauseMillis is same as MaxGCPauseMillis? I
didn't see such an indication in the code.<br>
</blockquote>
<br>
I actually ran with both collectors using
-XX:+PrintFlagsFinal and looked at the values printed. But
you find it in the code too. Both flags are declared in
globals.hpp as having max_uintx as default value:<br>
<br>
<br>
product(uintx, MaxGCPauseMillis,
max_uintx, \<br>
"Adaptive size policy maximum GC pause time goal
in msec, " \<br>
"or (G1 Only) the max. GC time per MMU time
slice") \<br>
<br>
product(uintx, MaxGCMinorPauseMillis,
max_uintx, \<br>
"Adaptive size policy maximum GC minor pause time
goal in msec") \<br>
<br>
<br>
<br>
<blockquote cite="mid:51158DCC.2020205@oracle.com"
type="cite">
<blockquote cite="mid:51152519.9030109@oracle.com"
type="cite"> <br>
But what happens if a customer is running with
-XX:MaxGCMinorPauseMillis=1000 but not setting
MaxGCPauseMillis on the command line? What are the
implications of your change for such runs? I don't think
it is very common. Just thinking. <br>
</blockquote>
For ParallelScavenge, customers should distinguish the two
flags.<br>
</blockquote>
<br>
Yes, they should, but my point was that if a customer was
just setting one of the flags their application might behave
differently now. And I was trying to understand in what way
it would behave different. But I actually think that your
change will not affect ParallelScavenge anyway. See below.<br>
<br>
<blockquote cite="mid:51158DCC.2020205@oracle.com"
type="cite"> For CMS, I wonder whether we have fully
implemented adaptive sizing policy. Jon, can you comment
on this?<br>
In either case, customers should be able to tell apart.<br>
</blockquote>
<br>
You are correct. Implementing adaptive sizing was never
completed for CMS so, it is turned off. Let's ignore CMS for
now.<br>
<br>
<blockquote cite="mid:51158DCC.2020205@oracle.com"
type="cite">
<blockquote cite="mid:51152519.9030109@oracle.com"
type="cite"> <br>
Also, a more common case is probably to run with only
-XX:MaxGCPauseMillis set on the command line. How will
the adaptive sizing behave differently compared to
before? <br>
</blockquote>
The fix only changed the value in the routine
GenCollectorPolicy::initialize_size_policy, which is
believed to be called within<br>
the routine of GenCollectedHeap::post_initialize(). I'm
not sure whether this routine can change sizing behaviors
somewhere.<br>
</blockquote>
<br>
post_initialize() is called from universe_post_init() for
all collectors. But only the CollectedHeaps that inherit
from GenCollectedHeap will be affected by your change.<br>
<br>
ParallelScavengeHeap does not inherit from GenCollectedHeap
and it implements its own post_initialize(), so I think it
is unaffected by your change.<br>
<br>
So, that's why I came to the conclusion that the code that
your are changing is actually not being used by anyone. If
this is correct I think your change is safe and for the
better. But I also think that we could possibly clean up the
code and get rid of some code paths. This should probably be
done as a separate change though.<br>
<br>
Bengt<br>
<br>
<br>
<br>
<blockquote cite="mid:51158DCC.2020205@oracle.com"
type="cite">
<blockquote cite="mid:51152519.9030109@oracle.com"
type="cite"> <br>
Looking closer at the code it looks like CMS and
ParallelScavenge are actually using
CMSAdaptiveSizePolicy and PSAdaptiveSizePolicy
respectively. Both of these already pass the correct
value to the constructor of the super class
AdaptiveSizePolicy. So, this bug is currently benign.
The code that you are changing is actually not being
used. Is this a correct conclusion? <br>
</blockquote>
See above. Whether the code I'm changing is being used
depends upon whether GenCollectedHeap::post_initialize()
is called anywhere. But I can't determine that right now.<br>
<br>
Other than that, the fix should correct the bug while
preserving everything else.<br>
<blockquote cite="mid:51152519.9030109@oracle.com"
type="cite"> <br>
Thanks, <br>
Bengt <br>
<br>
On 2/8/13 3:42 AM, Tao Mao wrote: <br>
<blockquote type="cite">8007764: Wrong initialized value
of max_gc_pause_sec for an instance of class
AdaptiveSizePolicy <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://jbs.oracle.com/bugs/browse/JDK-8007764">https://jbs.oracle.com/bugs/browse/JDK-8007764</a>
<br>
<br>
webrev: <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/8007764/webrev.00/">http://cr.openjdk.java.net/~tamao/8007764/webrev.00/</a>
<br>
<br>
changeset: <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
</body>
</html>