<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<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>
<br>
<blockquote cite="mid:5615381F.9010801@oracle.com" type="cite"> <br>
<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>
<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>
<br>
<blockquote cite="mid:5615381F.9010801@oracle.com" type="cite"> <br>
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>
<br>
<br>
<blockquote cite="mid:5615381F.9010801@oracle.com" type="cite">
<blockquote cite="mid:5614D1AA.5000105@oracle.com" type="cite"> <br>
In ConcurrentG1SampleThread::sample_young_list_rs_lengths() you
copied the code that does:<br>
<br>
if (g1p->adaptive_young_list_length()) {<br>
<br>
As far as I can tell this is always true, since the value for
adaptive_young_list_length() stems from
G1YoungGenSizer::_adaptive_size, which only seems to be set in
the constructor:<br>
<br>
G1YoungGenSizer::G1YoungGenSizer() : _sizer_kind(SizerDefaults),
_adaptive_size(true)<br>
<br>
Maybe this should be fixed separately, but it looks like it
should be cleaned up.<br>
</blockquote>
Ignored as requested.<br>
<br>
Thanks Bengt!<br>
<br>
- Derek<br>
</blockquote>
<br>
</body>
</html>