<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Thanks for the changes.<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8136678/webrev.0_to_1/src/share/vm/gc/g1/g1IHOPControl.cpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8136678/webrev.0_to_1/src/share/vm/gc/g1/g1IHOPControl.cpp.frames.html</a><br>
<br>
<pre><span class="new"> 185 static double percentage_of(double quantity, double base_quantity) {</span></pre>
<br>
looks a lot like<br>
<br>
share/vm/gc/g1/g1RemSetSummary.cpp<br>
<br>
static double percent_of(size_t numerator, size_t denominator) {<br>
<br>
Time to add a function to globalDefinitions?<br>
<br>
<br>
<br>
Minor issues.<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8136678/webrev.0_to_1/src/share/vm/gc/g1/g1IHOPControl.hpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8136678/webrev.0_to_1/src/share/vm/gc/g1/g1IHOPControl.hpp.frames.html</a><br>
<br>
You can ignore this but<br>
<br>
<pre><span class="new"> 103 // depending on predictions of current allocation rate and time periods between</span></pre>
<br>
might be better as<br>
<br>
<pre><span class="new"> 103 // <font color="#ff0000">based</font> on predictions of current allocation rate and time periods between</span></pre>
<br>
Something got jumbled up?<br>
<pre><span class="changed"> 117 // non-young gen occupancy <font color="#ff0000">compared against at the end of GC</font>, but we need to
Jon
</span></pre>
<br>
<div class="moz-cite-prefix">On 11/17/2015 01:24 AM, Thomas Schatzl
wrote:<br>
</div>
<blockquote cite="mid:1447752296.2288.34.camel@oracle.com"
type="cite">
<pre wrap="">Hi Jon,
thanks a lot for your comments, appreciated.
On Fri, 2015-11-13 at 13:03 -0800, Jon Masamitsu wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Thomas,
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8136678/webrev/src/share/vm/gc/g1/g1IHOPControl.hpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8136678/webrev/src/share/vm/gc/g1/g1IHOPControl.hpp.frames.html</a>
Can you add a comment describing what this is?
91 size_t _prev_unrestrained_young_size;
Why "recalculate" instead of just "calculate"? "calculate" has the
virtual of
being a little shorter name.
</pre>
</blockquote>
<pre wrap="">
Fixed.
</pre>
<blockquote type="cite">
<pre wrap="">95 // Updates _current_threshold according to internal state.
96 void recalculate();
This says that you want the target_occupancy to be at the maximum value
that will still allow young gen sizes as set with G1ReservePercen? That
seems
a bit aggressive to me since it is the value that will be used until
there is
enough data to create a better estimate. Maybe arbitrarily add an
extra 20%
to safe_heap_percentage?
1257 if (safe_heap_percentage < 100) {
1258 target_occupancy = G1CollectedHeap::heap()->max_capacity() * (100.0
- safe_heap_percentage) / 100.0;
1259 }
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8136678/webrev/src/share/vm/gc/g1/g1IHOPControl.cpp.frames.html">http://cr.openjdk.java.net/~tschatzl/8136678/webrev/src/share/vm/gc/g1/g1IHOPControl.cpp.frames.html</a>
</pre>
</blockquote>
<pre wrap="">
I fixed that by moving this calculation into the policy to make clear
that this _target_occupancy is an internal value. I would have needed
this refactoring anyway for JDK-8140777.
(And it certainly looks better.)
</pre>
<blockquote type="cite">
<pre wrap="">Add a flag in place of the "2" just to make experimentation easier?
133 return (_marking_times_s.num() > 2) && (_allocation_rate_s.num() > 2);
</pre>
</blockquote>
<pre wrap="">
Added.
</pre>
<blockquote type="cite">
<pre wrap="">
Do you still want the comment at the end?
244 size_t const settled_ihop3 = 0; // target_size - (young_size +
alloc_amount2/alloc_time2 * marking_time2);
</pre>
</blockquote>
<pre wrap="">
Removed. Copy&paste error :)
</pre>
<blockquote type="cite">
<pre wrap="">Rest looks good.
Jon
</pre>
</blockquote>
<pre wrap="">
New webrevs:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8136678/webrev.0_to_1/">http://cr.openjdk.java.net/~tschatzl/8136678/webrev.0_to_1/</a> (diff)
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8136678/webrev.1/">http://cr.openjdk.java.net/~tschatzl/8136678/webrev.1/</a> (full)
Thanks,
Thomas
</pre>
</blockquote>
<br>
</body>
</html>