<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Thanks Thomas,<br>
<br>
On 10/21/15 4:51 AM, Thomas Schatzl wrote:<br>
</div>
<blockquote cite="mid:1445417487.2246.17.camel@oracle.com"
type="cite">
<pre wrap="">Hi,h
On Tue, 2015-10-20 at 15:56 -0400, Derek White wrote:
</pre>
<blockquote type="cite">
<pre wrap="">4th Webrev for review please:
This version includes the cleanups suggested by Per.
RFE: JDK-8138920 Refactor the sampling thread from
ConcurrentG1RefineThread
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8138920/webrev.04/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.04/</a>
jprt: waiting for results (which might take a long time given current
jprt status).
Thanks for looking!
</pre>
</blockquote>
<pre wrap="">
Looks good overall. Some minor comments:
- I am okay with deferring the move of the sample thread to another CR.
Is there a CR for that already?</pre>
</blockquote>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<a class="issue-link" data-issue-key="JDK-8140255"
href="https://bugs.openjdk.java.net/browse/JDK-8140255"
id="key-val" rel="4850319">JDK-8140255 Move the management of
G1YoungRemSetSamplingThread from ConcurrentG1Refine<br>
</a><br>
<blockquote cite="mid:1445417487.2246.17.camel@oracle.com"
type="cite">
<pre wrap="">- the comment for ConcurrentG1Refine::sampling_thread() might need some
elaboration.</pre>
</blockquote>
I agree. :-) If you have any suggestions...<br>
<blockquote cite="mid:1445417487.2246.17.camel@oracle.com"
type="cite">
<pre wrap="">- also add some description of the purpose of the new
G1YoungListSampleThread class. </pre>
</blockquote>
You are assuming that I understand what this code is really doing
(and why) :-)<br>
<br>
OK, in order to understand this better I really needed a definition
of "MMU". Surprisingly I could find no definition of MMU in the
source code. Although that seems to be a more major deficiency in
our documentation, I'd like to finish 8138920 someday. <span
class="issue-link">See "</span><a class="issue-link"
data-issue-key="JDK-8140251"
href="https://bugs.openjdk.java.net/browse/JDK-8140251"
id="key-val" rel="4850312">JDK-8140251</a> Define the G1 term MMU
somewhere in the source code".<br>
<br>
Still looking into how this all fits together. I ask myself - How is
this related to
ConcurrentMarkThread::handle_adaptive_young_list_length()? Does that
really have to do with marking? Where are other parts of this
functionality hidden?
<meta http-equiv="content-type" content="text/html; charset=utf-8">
What is that beautiful house?
<meta http-equiv="content-type" content="text/html; charset=utf-8">
Where does that highway go to?
<meta http-equiv="content-type" content="text/html; charset=utf-8">
Am I right?...Am I wrong?
<meta http-equiv="content-type" content="text/html; charset=utf-8">
My God!...What have I done?! <br>
<br>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
Same as it ever was... Still looking...<br>
<blockquote cite="mid:1445417487.2246.17.camel@oracle.com"
type="cite">
<pre wrap="">I am okay with the name, it could be more
specific like G1YoungRemSetSamplingThread. I will let you decide whether
to keep it.</pre>
</blockquote>
OK, changed.<br>
<blockquote cite="mid:1445417487.2246.17.camel@oracle.com"
type="cite">
<pre wrap="">- please rename
ConcurrentMarkThread::handle_adaptive_young_list_length() to something
more meaningful. The method delays the current thread so that the next
expected pauses observe the MMU. (I think it is really good to extract
this into a method!)
So something like observe_mmu() or delay_thread_to_keep_mmu() would be
imo more appropriate. These are just suggestions I came up within a few
seconds though...</pre>
</blockquote>
Those sound better, but still looking...<br>
<blockquote cite="mid:1445417487.2246.17.camel@oracle.com"
type="cite">
<pre wrap="">
- please also add some description of what these new methods do if not
really obvious. E.g. that handle_adaptive_young_list_length() method
does not qualify for obvious :)</pre>
</blockquote>
Lots of non-obvious here. I found it pretty confusing that the
variable "adaptive_young_list_length" is not actually a length, but
a boolean!<br>
<blockquote cite="mid:1445417487.2246.17.camel@oracle.com"
type="cite">
<pre wrap=""> I saw that the change fixes existing
comments, but they are few and far between.
Thanks for handling this issue,
Thomas
</pre>
</blockquote>
<br>
OK, thanks Thomas. I'll get a new webrev out after some more
research.<br>
<br>
- Derek<br>
<br>
</body>
</html>