RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread
Per Liden
per.liden at oracle.com
Thu Oct 22 07:16:00 UTC 2015
Hi,
On 2015-10-21 19:03, Derek White wrote:
> Hi Per,
>
> On 10/21/15 6:42 AM, Per Liden wrote:
>> 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.
>
> OK.
>>
>> 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?
> Deleted tests as suggested by Thomas.
>> 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.
>
> OK.
>> concurrentG1RefineThread.hpp
>> ----------------------------
>>
>> 71 virtual void run_service();
>> 72 virtual void stop_service();
>>
>> Please make these non-virtual.
>
> OK. I'll revirtualize in 8140257
> <https://bugs.openjdk.java.net/browse/JDK-8140257>.
Sounds good.
>>
>>
>> g1YoungListSampleThread.hpp
>> ---------------------------
>>
>> 30 class G1YoungListSampleThread: public ConcurrentGCThread {
>> 31 Monitor* _monitor;
>>
>> Please add a "private:" here.
>
> OK. BTW, I noticed pretty inconsistent indentation of "private:" in the
> G1 code, especially right after the class name. Some cases are zero
> indent and some are one (1!). I'm assuming that zero is correct.
If we go by what the style guide says my interpretation is that zero
indent is more correct here. That's also what I personally prefer,
unless I'm just doing a small fix in which case I use whatever the class
is currently using.
/Per
>> 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.
>
> OK.
>>
>> cheers,
>> /Per
>>
> Thanks Per!
>
> - Derek
>>>
>>> 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