<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
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>
<br>
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>
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>
<br>
You write "and also fixes a P4 bug". Which bug is that?<br>
<br>
<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>
<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:56140A14.2030703@oracle.com" type="cite"> <br>
Testing: jprt<br>
<br>
Perf testing: in progress...<br>
<br>
<br>
Thanks!<br>
<br>
- Derek<br>
<br>
<br>
<br>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
</blockquote>
<br>
</body>
</html>