<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>
Hi Tao,<br>
<br>
First, I realize that I am very late with this feedback. Sorry for
that.<br>
<br>
On 3/5/13 8:43 AM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:5135A213.6030001@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Bengt,<br>
<br>
As I understand (since I've investigated the resizing issue for
a while) and, in fact, if you look at the comments you quoted,
the routine compute_generations_free_space() just does the
following:<br>
<br>
input(stats of young & old spaces) -> output(calculated
eden & promo sizes).<br>
</div>
</blockquote>
<br>
I disagree. The method has lots of side effects. It does not just
take some input and produce some output. It updates the global state
in many ways. For example through the statistics gathering with at
least 10-20 instance variables such as:<br>
<br>
_avg_base_footprint <br>
_avg_young_live<br>
_avg_eden_live<br>
_avg_old_live<br>
_decide_at_full_gc<br>
_change_old_gen_for_maj_pauses<br>
_change_young_gen_for_maj_pauses<br>
<br>
..and more<br>
<br>
It also logs messages associated with PrintAdaptiveSizePolicy.<br>
<br>
In fact I would consider the calculation of free size as a very
small part of what the method does.<br>
<br>
The name compute_generations_free_space() indicates to me that it
would be a simple input->output method just as you suggested. But
in that case I would expect that it would be safe to call it several
times in a row. This is not the case with the current
implementation. We can only call it exactly once at the exact right
moment in the during GC.<br>
<br>
So, again, I'm fine with leaving the name untouched as
compute_generation_free_space(). But if we should change it I think
we should make a more meaningful change than what you proposed.<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:5135A213.6030001@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
I would say, it's valid to name it this way.<br>
<br>
In fact, in another CR (8007763, one of the series of CR's you
asked me to split out), we would like to split
compute_generations_free_space() into compute_eden* and
compute_old*. This also validates the renaming here.<br>
<br>
For commenting in psGCAdaptivePolicyCounters.hpp, the comment
leads a series of routines to update stats for
compute_generations_free_space(). It has some value to keep it.
But, I'm ok either way for the choice here doesn't matter too
much.<br>
<br>
Thanks.<br>
Tao<br>
<br>
<br>
On 2013/3/4 22:56, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:51359721.5030508@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>
The method
PSAdaptiveSizePolicy::compute_generation_free_space() is a
monster method. It does all kinds of things. Its comment kind
of reveals this:<br>
<br>
339 // Calculates optimial free space sizes for both the
old and young<br>
340 // generations. Stores results in _eden_size and
_promo_size.<br>
341 // Takes current used space in all generations as
input, as well<br>
342 // as an indication if a full gc has just been
performed, for use<br>
343 // in deciding if an OOM error should be thrown.<br>
<br>
So, I think that if we should rename it we should give it a
name that reflects this. The current name is not good, but I
don't think compute_generations_free_space() is much better. I
would suggest to either leave it unchanged or change to a more
descriptive name.<br>
<br>
Maybe gether_adapitve_size_statistics() or
update_adaptive_size_values(). Not very happy with those names
either, but at least they indicate that ther eis more than
just free space calculations in there.<br>
<br>
<br>
In psGCAdaptivePolicyCounters.hpp you updated a method name
that has been commented out. I would suggest removing it
rather than updating it.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
On 3/5/13 1:46 AM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:51354057.1070308@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Oops...sent to open list.<br>
<br>
Thanks.<br>
Tao<br>
<div class="moz-cite-prefix"><br>
On 3/4/2013 4:38 PM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:51353E78.1040700@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
This is the proposed final webrev, with copyright dated
appropriately.<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/8007762/webrev.02/">http://cr.openjdk.java.net/~tamao/8007762/webrev.02/</a><br>
<br>
Jon,<br>
A patch is rendered below. Please take the patch and help
push the changeset.<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/8007762/webrev.02/8007762RenameMethodsInSizePolicy.patch">http://cr.openjdk.java.net/~tamao/8007762/webrev.02/8007762RenameMethodsInSizePolicy.patch</a><br>
<br>
Thank you.<br>
Tao<br>
<br>
On 2/26/13 8:37 AM, Jon Masamitsu wrote:
<blockquote cite="mid:512CE4C5.6080101@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Thanks. <br>
<br>
Looks good.<br>
<br>
Jon<br>
<br>
On 02/25/13 11:09, Tao Mao wrote:
<blockquote cite="mid:512BB6E6.6060503@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
A new webrev is updated. <br>
Thanks.<br>
Tao<br>
<br>
<div class="moz-cite-prefix">On 2/18/2013 7:46 AM, Jon
Masamitsu wrote:<br>
</div>
<blockquote cite="mid:51224CBF.8050705@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Tao,<br>
<br>
Why this change from resize_old_gen() to<br>
resize_tenured_gen()? There are many<br>
variable names and comments referring to<br>
"old gen" still in ParallelScavengeHeap.<br>
</blockquote>
Reverted.<br>
<blockquote cite="mid:51224CBF.8050705@oracle.com"
type="cite"> <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/8007762/webrev.00/src/share/vm/gc_implementation/parallelScavenge/parallelScavengeHeap.cpp.udiff.html">http://cr.openjdk.java.net/~tamao/8007762/webrev.00/src/share/vm/gc_implementation/parallelScavenge/parallelScavengeHeap.cpp.udiff.html</a><br>
<br>
Jon<br>
<br>
On 02/14/13 11:18, Tao Mao wrote:
<blockquote cite="mid:511D388A.4010700@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
8007762: Rename a bunch of methods in size policy
across collectors<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://jbs.oracle.com/bugs/browse/JDK-8007762">https://jbs.oracle.com/bugs/browse/JDK-8007762</a><br>
<br>
webrev: <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/8007762/webrev.00/">http://cr.openjdk.java.net/~tamao/8007762/webrev.00/</a><br>
<br>
changeset:<br>
The motivation of this change is associated with CR
7098155 (<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://jbs.oracle.com/bugs/browse/JDK-7098155">https://jbs.oracle.com/bugs/browse/JDK-7098155</a>).
For this, we need to split
compute_generations_free_space into two routine (one
for compute_eden_space_size and the other for
compute_tenured_generation_free_space) in order to
save some overhead when we want to compute only one
of the two. The next step is to split
PSAdaptiveSizePolicy::compute_generations_free_space(),
which will be resolved in CR 8007763.<br>
<br>
To unify the naming, a bunch of routines are renamed
here.<br>
(1) compute_*: <br>
compute_survivor_space_size_and_threshold() <br>
compute_generations_free_space() =
compute_eden_space_size() +
compute_tenured_generation_free_space()<br>
(2) resize_*_gen: <br>
resize_young_gen() <br>
resize_tenured_gen() <br>
(3) rename judging routine resize_geneneration() to
need_resize() to avoid ambiguity<span style="color:
rgb(0, 0, 0); font-family: Arial, FreeSans,
Helvetica, sans-serif; font-size: 13px;
font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing: normal;
line-height: 17px; orphans: 2; text-align: start;
text-indent: 0px; text-transform: none;
white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-size-adjust: auto;
-webkit-text-stroke-width: 0px; background-color:
rgb(255, 255, 255); display: inline !important;
float: none;"></span><br>
<br>
testing:<br>
purely renaming; passed JPRT<br>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>