<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Another call for review:<br>
<br>
2nd webrev:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/8138920/webrev.02/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.02/</a><br>
<br>
See changes and comments below:<br>
<br>
On 10/8/15 2:47 AM, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:5616118F.2060208@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<br>
Hi Derek,<br>
<br>
<div class="moz-cite-prefix">On 2015-10-07 17:19, Derek White
wrote:<br>
</div>
<blockquote cite="mid:5615381F.9010801@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Bengt,<br>
<br>
On 10/7/15 4:02 AM, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:5614D1AA.5000105@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Derek,<br>
<br>
Thanks for fixing this!<br>
<br>
<div class="moz-cite-prefix">On 2015-10-06 19:51, Derek White
wrote:<br>
</div>
<blockquote cite="mid:56140A14.2030703@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
Refactor and cleanup the G1 concurrent thread classes:<br>
- Pull out a sampling thread class (now
ConcurrentG1SampleThread) from ConcurrentG1RefineThread.<br>
- Factor out an abstract base class
ConcurrentG1ServiceThread that is used by:<br>
- ConcurrentG1RefineThread<br>
- ConcurrentG1SampleThread<br>
- ConcurrentMarkThread<br>
- Made the handling of the "primary" refinement thread more
explicit.<br>
- Updated obsolete and confusing comments<br>
<br>
This is tech debt that also will allow disabling concurrent
refinement (if desired) and also fixes a P4 bug.<br>
Patch started by Thomas and improved and/or mangled myself.<br>
<br>
RFE:
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<a moz-do-not-send="true" class="issue-link"
data-issue-key="JDK-8138920"
href="https://bugs.openjdk.java.net/browse/JDK-8138920"
id="key-val" rel="4848143">JDK-8138920</a> Refactor the
sampling thread from ConcurrentG1RefineThread<br>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/8138920/webrev.01/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.01/</a><br>
</blockquote>
<br>
Overall this looks really good to me. <br>
<br>
Some comments:<br>
<br>
No one seems to depend on
ConcurrentG1ServiceThread::vtime_accum(). All uses have the
specific subclass available. So, I don't think the pure
virtual declaration in ConcurrentG1ServiceThread is needed.
I'd just remove that and also make the corresponding methods
in the subclasses non-virtual.<br>
</blockquote>
<br>
OK. At some point we need to systematic rewrite of timing, but
that can wait.<br>
</blockquote>
<br>
Quite agree. <br>
<br>
<blockquote cite="mid:5615381F.9010801@oracle.com" type="cite">
<blockquote cite="mid:5614D1AA.5000105@oracle.com" type="cite">
That more or less only leaves the run() and stop() methods in
ConcurrentG1ServiceThread. It is kind of nice for the
subclasses to get help with this, but I wonder if it is not
possible to improve the ConcurrentGCThread class to help with
this instead. Basically I guess I am a little unsure if the
extra class ConcurrentG1ServiceThread is really needed.<br>
<br>
</blockquote>
I'll look at ConcurrentGCThread to see how well it could cover
these cases.<br>
</blockquote>
<br>
Thanks. I think it is worth a try. If it doesn't turn out well we
can keep the intermediate class, but I think it is worth
exploring.<br>
</blockquote>
<br>
I looked at pushing ConcurrentServiceThread up into
ConcurrentGCThread, but things got a little complicated.
ConcurrentG1RefineThread, ConcurrentMarkThread, and
ConcurrentSampleThread have a very "regularized" implementation of
the "termination protocol". G1StringDedupThread is slightly off from
this, and ConcurrentMarkSweepThread more so. Pushing the shared
code up into ConcurrentGCThread but not using it in
G1StringDedupThread and ConcurrentMarkSweepThread seems confusing.<br>
<br>
There's a tension between providing a framework that handles all
shared factorizable code, but can become a straight jacket for
future code, and everyone doing everything separately and
differently. This webrev is somewhere in the middle. Some of the
changes between webrev.01 and .02 are to make the duplicated code
more similar, even though it's not shared.<br>
<br>
<blockquote cite="mid:5616118F.2060208@oracle.com" type="cite">
<blockquote cite="mid:5615381F.9010801@oracle.com" type="cite">
<blockquote cite="mid:5614D1AA.5000105@oracle.com" type="cite">
Naming. The naming in G1 is a bit inconsistent. Some files and
classes are prefixed with G1 and some are not. But if they are
called something with G1 it is normally a prefix. So, I would
prefer the new classes to be called G1Concurrent* instead of
ConcurrentG1*.<br>
</blockquote>
<br>
So we have:<br>
- ConcurrentG1RefineThread<br>
- ConcurrentMarkThread<br>
<br>
And I added:<br>
- ConcurrentG1SampleThread<br>
- ConcurrentG1ServiceThread<br>
<br>
And maybe I'm removing ConcurrentG1ServiceThread. In that case
I'd be inclined to rename:<br>
ConcurrentG1SampleThread => ConcurrentSampleThread </blockquote>
<br>
Sounds good. The ConcurrentG1Refine* classes are really the only
oddly named G1 classes, so I think it is better not to let that
naming spread.<br>
</blockquote>
This version includes the class renaming.
<blockquote cite="mid:5616118F.2060208@oracle.com" type="cite"> <br>
<blockquote cite="mid:5615381F.9010801@oracle.com" type="cite">
<blockquote cite="mid:5614D1AA.5000105@oracle.com" type="cite">
You write "and also fixes a P4 bug". Which bug is that?<br>
</blockquote>
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<a moz-do-not-send="true" class="issue-link"
data-issue-key="JDK-8136856"
href="https://bugs.openjdk.java.net/browse/JDK-8136856"
id="key-val" rel="4845182">JDK-8136856</a> G1 makes two
concurrent refinement threads with -XX:G1ConcRefinementThreads=1<br>
(because the sampling thread "is-a" concurrent refinement
thread.<br>
</blockquote>
<br>
Ah. I see. Makes sense. Thanks.<br>
<br>
But it is still not possible to turn refinement off by setting
-XX:G1ConcRefinementThreads=0 since that is considered the
default, right?<br>
</blockquote>
<br>
I'm not sure about this. If I recall correctly, Thomas implied that
it was hard to disable concurrent refinement without disabling the
sampling thread too.<br>
<br>
<blockquote cite="mid:5616118F.2060208@oracle.com" type="cite">
<blockquote cite="mid:5615381F.9010801@oracle.com" type="cite"> I
cant' recall how to mark a bug as blocking another bug.<br>
</blockquote>
<br>
You add a link (More->Link) to the other bug and choose "block"
or "is blocked by".<br>
<br>
Thanks,<br>
Bengt<br>
</blockquote>
Thanks for the tip!<br>
<br>
- Derek<br>
<br>
</body>
</html>