RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread
Derek White
derek.white at oracle.com
Fri Oct 23 17:43:31 UTC 2015
RFR 5.
This responds to Per's last comments and Thomas's suggestions for better
methods names and comments.
Webrev: http://cr.openjdk.java.net/~drwhite/8138920/webrev.05/
Webrev 05 vs 04:
http://cr.openjdk.java.net/~drwhite/8138920/webrev.05.vs.04/
RFE: JDK-8138920 <https://bugs.openjdk.java.net/browse/JDK-8138920>
Refactor the sampling thread from ConcurrentG1RefineThread
Thanks,
- Derek
On 10/22/15 3:16 AM, Per Liden wrote:
> 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