RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread
Derek White
derek.white at oracle.com
Tue Oct 20 19:56:58 UTC 2015
4th Webrev for review please:
This version includes the cleanups suggested by Per.
RFE: JDK-8138920 <https://bugs.openjdk.java.net/browse/JDK-8138920>
Refactor the sampling thread from ConcurrentG1RefineThread
Webrev: http://cr.openjdk.java.net/~drwhite/8138920/webrev.04/
jprt: waiting for results (which might take a long time given current
jprt status).
Thanks for looking!
- Derek
On 10/20/15 1:41 PM, Derek White wrote:
> Thanks for the review Per! Comments below...
>
> On 10/20/15 2:56 AM, Per Liden wrote:
>> Hi Derek,
>>
>> On 2015-10-19 22:08, Derek White wrote:
>>> Thanks Bengt (and Per indirectly). Comments below.
>>>
>>> On 10/19/15 3:17 PM, Bengt Rutisson wrote:
>>>> Hi Derek,
>>>>
>>>> On 2015-10-17 03:32, Derek White wrote:
>>>>> 3rd Webrev for review please:
>>>>>
>>>>> 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.
>>>>>
>>>>>
>>>>> RFE: JDK-8138920 <https://bugs.openjdk.java.net/browse/JDK-8138920>
>>>>> Refactor the sampling thread from ConcurrentG1RefineThread
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~drwhite/8138920/webrev.03/
>>>>
>>>> I think this version is much good.
>>>>
>>>> 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.
>>>>
>>>> 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.
>>> 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).
>>
>> I'm ok with keeping it like this if the ConcurrentGCThread cleanup is
>> something we'll get to soon-ish.
>>
>> concurrentSampleThread.hpp/cpp
>> ------------------------------
>>
>> * I'd suggest we name the new thread it G1SampleThread (or maybe
>> G1YoungListSampleThread, or something similar). The fact that it's
>> concurrent doesn't seem important enough to make it part of the name,
>> better to try to say something about what it does.
> OK.
>
>> * sleepBeforeNextCycle() should be sleep_before_next_cycle(). I know
>> concurrentMarkThread and maybe some other thread also has a
>> sleepBeforeNextCycle() and I think that should be changed too at some
>> point (not saying you need to do that in this change).
>>
> OK
>> * The protected members should be private, i.e. :
>>
>> 30 class ConcurrentSampleThread: public ConcurrentGCThread {
>> 31 protected:
>> 32 Monitor* _monitor;
>> 33
>> 34 void sample_young_list_rs_lengths();
>> 35
>> 36 void run_service();
>> 37 void stop_service();
>> 38
>> 39 void sleepBeforeNextCycle();
>> 40
>> 41 double _vtime_accum; // Accumulated virtual time.
> OK
>> * I could be wrong here, but I can see any immediate need to include
>> these, or?
>>
>> 30 #include "memory/resourceArea.hpp"
>> 31 #include "runtime/handles.inline.hpp"
>>
> Not likely. I'll Check.
>
>> concurrentG1Refine.hpp/cpp
>> --------------------------
>>
>> * As Thomas mentioned, I think the sample thread ownership should
>> move from the ConcurrentG1Refine to to G1CollectedHeap. That seems to
>> make more sense, no?
>
> Yes, but I'd like to leave that as a separate fix. One that actually
> allows concurrent refinement to be disabled. It would take me a bit to
> figure out how to that correctly (and test).
>
> Thanks again, new webrev in a bit...
>
> - Derek
>>
>> cheers,
>> /Per
>>
>>>
>>> - Derek
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>>>
>>>>> Passed jprt (finally!).
>>>>>
>>>>> Thanks for looking!
>>>>>
>>>>> - Derek
>>>>>
>>>>> On 10/9/15 12:01 PM, Bengt Rutisson wrote:
>>>>>>
>>>>>> Hi Derek,
>>>>>>
>>>>>> Comments inlined.
>>>>>>
>>>>>> On 2015-10-09 00:29, Derek White wrote:
>>>>>>> Another call for review:
>>>>>>>
>>>>>>> 2nd webrev:
>>>>>>> http://cr.openjdk.java.net/~drwhite/8138920/webrev.02/
>>>>>>>
>>>>>>> See changes and comments below:
>>>>>>>
>>>>>>> On 10/8/15 2:47 AM, Bengt Rutisson wrote:
>>>>>>>>
>>>>>>>> Hi Derek,
>>>>>>>>
>>>>>>>> On 2015-10-07 17:19, Derek White wrote:
>>>>>>>>> Hi Bengt,
>>>>>>>>>
>>>>>>>>> On 10/7/15 4:02 AM, Bengt Rutisson wrote:
>>>>>>>>>> Hi Derek,
>>>>>>>>>>
>>>>>>>>>> Thanks for fixing this!
>>>>>>>>>>
>>>>>>>>>> On 2015-10-06 19:51, Derek White wrote:
>>>>>>>>>>> Refactor and cleanup the G1 concurrent thread classes:
>>>>>>>>>>> - Pull out a sampling thread class (now
>>>>>>>>>>> ConcurrentG1SampleThread) from ConcurrentG1RefineThread.
>>>>>>>>>>> - Factor out an abstract base class ConcurrentG1ServiceThread
>>>>>>>>>>> that is used by:
>>>>>>>>>>> - ConcurrentG1RefineThread
>>>>>>>>>>> - ConcurrentG1SampleThread
>>>>>>>>>>> - ConcurrentMarkThread
>>>>>>>>>>> - Made the handling of the "primary" refinement thread more
>>>>>>>>>>> explicit.
>>>>>>>>>>> - Updated obsolete and confusing comments
>>>>>>>>>>>
>>>>>>>>>>> This is tech debt that also will allow disabling concurrent
>>>>>>>>>>> refinement (if desired) and also fixes a P4 bug.
>>>>>>>>>>> Patch started by Thomas and improved and/or mangled myself.
>>>>>>>>>>>
>>>>>>>>>>> RFE: JDK-8138920
>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8138920> Refactor the
>>>>>>>>>>> sampling thread from ConcurrentG1RefineThread
>>>>>>>>>>>
>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~drwhite/8138920/webrev.01/
>>>>>>>>>>
>>>>>>>>>> Overall this looks really good to me.
>>>>>>>>>>
>>>>>>>>>> Some comments:
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> OK. At some point we need to systematic rewrite of timing, but
>>>>>>>>> that can wait.
>>>>>>>>
>>>>>>>> Quite agree.
>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>> I'll look at ConcurrentGCThread to see how well it could cover
>>>>>>>>> these cases.
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> I see. Thanks for investigating it!
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>>>
>>>>>>>>>> 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*.
>>>>>>>>>
>>>>>>>>> So we have:
>>>>>>>>> - ConcurrentG1RefineThread
>>>>>>>>> - ConcurrentMarkThread
>>>>>>>>>
>>>>>>>>> And I added:
>>>>>>>>> - ConcurrentG1SampleThread
>>>>>>>>> - ConcurrentG1ServiceThread
>>>>>>>>>
>>>>>>>>> And maybe I'm removing ConcurrentG1ServiceThread. In that case
>>>>>>>>> I'd be inclined to rename:
>>>>>>>>> ConcurrentG1SampleThread => ConcurrentSampleThread
>>>>>>>>
>>>>>>>> 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.
>>>>>>> This version includes the class renaming.
>>>>>>
>>>>>> Thanks for fixing this.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>>>>
>>>>>>>>>> You write "and also fixes a P4 bug". Which bug is that?
>>>>>>>>> JDK-8136856 <https://bugs.openjdk.java.net/browse/JDK-8136856> G1
>>>>>>>>> makes two concurrent refinement threads with
>>>>>>>>> -XX:G1ConcRefinementThreads=1
>>>>>>>>> (because the sampling thread "is-a" concurrent refinement thread.
>>>>>>>>
>>>>>>>> Ah. I see. Makes sense. Thanks.
>>>>>>>>
>>>>>>>> But it is still not possible to turn refinement off by setting
>>>>>>>> -XX:G1ConcRefinementThreads=0 since that is considered the
>>>>>>>> default, right?
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> I'm thinking about this code in Arguments::set_g1_gc_flags():
>>>>>>
>>>>>> if (G1ConcRefinementThreads == 0) {
>>>>>> FLAG_SET_DEFAULT(G1ConcRefinementThreads, ParallelGCThreads);
>>>>>> }
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Thanks,
>>>>>> Bengt
>>>>>>
>>>>>>>
>>>>>>>>> I cant' recall how to mark a bug as blocking another bug.
>>>>>>>>
>>>>>>>> You add a link (More->Link) to the other bug and choose "block" or
>>>>>>>> "is blocked by".
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Bengt
>>>>>>> Thanks for the tip!
>>>>>>>
>>>>>>> - Derek
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20151020/7336b330/attachment.htm>
More information about the hotspot-gc-dev
mailing list