<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Hi again,<br>
<br>
A short update on my comment about adaptive size inlined below.<br>
<br>
<div class="moz-cite-prefix">On 2015-10-07 10:02, 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">
<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>
</blockquote>
<br>
Scratch that. I missed that the G1YoungGenSizer constructor resets
the _adaptive_size value after it has been initialized in the
initialization list. So, the value can actually be false.<br>
<br>
Sorry about the noise.<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:5614D1AA.5000105@oracle.com" type="cite"> <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>
</blockquote>
<br>
</body>
</html>