RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread
Per Liden
per.liden at oracle.com
Wed Oct 21 10:42:32 UTC 2015
Hi Derek,
On 2015-10-20 21:56, Derek White wrote:
> 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/
Looks good over all. Just a few minor things I noticed.
concurrentG1Refine.cpp
----------------------
92 vm_shutdown_during_initialization("Could not create
ConcurrentSampleThread");
Should be "G1YoungListSampleThread" now.
160 void ConcurrentG1Refine::print_worker_threads_on(outputStream* st)
const {
161 for (uint i = 0; i < _n_worker_threads; ++i) {
162 _threads[i]->print_on(st);
163 st->cr();
164 }
165 _sample_thread->print_on(st);
166 st->cr();
You didn't introduce this, but shouldn't there be a if (_threads !=
NULL) here around this, similar to the other functions which access
_threads?
concurrentG1RefineThread.cpp
----------------------------
199 void ConcurrentG1RefineThread::stop_service() {
200 {
201 MutexLockerEx x(_monitor, Mutex::_no_safepoint_check_flag);
202 _monitor->notify();
203 }
204 }
The extra scope here seems superfluous.
concurrentG1RefineThread.hpp
----------------------------
71 virtual void run_service();
72 virtual void stop_service();
Please make these non-virtual.
g1YoungListSampleThread.hpp
---------------------------
30 class G1YoungListSampleThread: public ConcurrentGCThread {
31 Monitor* _monitor;
Please add a "private:" here.
g1YoungListSampleThread.cpp
---------------------------
59 _monitor = new Monitor(Mutex::nonleaf,
60 "Sample thread monitor",
61 true,
62 Monitor::_safepoint_check_never);
It looks like _monitor could be a value member instead of a pointer, to
avoid the extra allocation here.
66 set_name("G1 Sample");
This should probably be updated to reflect the new name.
cheers,
/Per
>
> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
More information about the hotspot-gc-dev
mailing list