<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 Bengt (and Per indirectly).
Comments below.<br>
<br>
On 10/19/15 3:17 PM, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:562541E2.5020406@oracle.com" type="cite"> Hi
Derek,<br>
<br>
<div class="moz-cite-prefix">On 2015-10-17 03:32, Derek White
wrote:<br>
</div>
<blockquote cite="mid:5621A513.9040803@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">3rd Webrev for review please:<br>
<br>
This version does away with the common abstract base case
ConcurrentServiceThread, and pushes the code down to the
concrete classes. This may get cleaned up in a later fix to
ConcurrentGCThread.<br>
<br>
<br>
RFE: <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.03/">http://cr.openjdk.java.net/~drwhite/8138920/webrev.03/</a><br>
</div>
</blockquote>
<br>
I think this version is much good.<br>
<br>
I talked a bit to Per about the latest webrev and he pointed out
that we don't really need the run_service()/stop_service()
abstraction now that the threads implement all this themselves.
The change would be smaller if we left that change out.<br>
<br>
I'll leave it up to you if you want to keep it in there or not. Do
you plan on following this work up by starting to work onÂ
JDK-8138920 pretty soon? In that case I think the
run_service()/stop_service() abstraction will have to be handled
there anyway which makes it a smaller issue now in my opinion.<br>
</blockquote>
I went back and forth about including run_service()/stop_service(),
but decided that the refactoring had already been done, and I'd like
to push run()/stop() up to ConcurrentGCThread like we talked about
soon. There are a few extra complications that will also need to be
handled (like G1StringDedupThread already has a static stop()
method).<br>
<br>
 - Derek<br>
<blockquote cite="mid:562541E2.5020406@oracle.com" type="cite"> <br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:5621A513.9040803@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
Passed jprt (finally!).<br>
<br>
Thanks for looking!<br>
<br>
 - Derek<br>
<br>
On 10/9/15 12:01 PM, Bengt Rutisson wrote:<br>
</div>
<blockquote cite="mid:5617E4C6.3040501@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<br>
Hi Derek,<br>
<br>
Comments inlined.<br>
<br>
<div class="moz-cite-prefix">On 2015-10-09 00:29, Derek White
wrote:<br>
</div>
<blockquote cite="mid:5616EE55.4010902@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Another call for review:<br>
<br>
2nd webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/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>
</blockquote>
<br>
I see. Thanks for investigating it!<br>
<br>
I think I agree with Per, though. The value of
ConcurrentServiceThread in its current form is IMHO not really
worth the extra complexity of having it. I would prefer to
just duplicate the code in ConcurrentG1RefineThread,
ConcurrentMarkThread, and ConcurrentSampleThread for now and
then have a separate change to try to clean this part up.<br>
<br>
<blockquote cite="mid:5616EE55.4010902@oracle.com" type="cite">
<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>
<br>
Thanks for fixing this.<br>
<br>
If we do decide to keep ConcurrentServiceThread around I think
it would be better to move ConcurrentSampleThread into its own
file. It is a separate enough entity to warrant its own file I
think.<br>
<br>
<blockquote cite="mid:5616EE55.4010902@oracle.com" type="cite">
<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>
</blockquote>
<br>
I'm thinking about this code in Arguments::set_g1_gc_flags():<br>
<br>
 if (G1ConcRefinementThreads == 0) {<br>
   FLAG_SET_DEFAULT(G1ConcRefinementThreads,
ParallelGCThreads);<br>
 }<br>
<br>
Could we change that, so that you could turn off refinement by
setting -XX:G1ConcRefinementThreads=0 ? It should probably be
handled as a separate fix though.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote cite="mid:5616EE55.4010902@oracle.com" type="cite">
<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>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>