RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread
Derek White
derek.white at oracle.com
Tue Oct 20 17:41:19 UTC 2015
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
>>>>>>
>>>>>
>>>>
>>>
>>
More information about the hotspot-gc-dev
mailing list