<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<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>
</body>
</html>