RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

Derek White derek.white at oracle.com
Mon Oct 19 20:08:36 UTC 2015 

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).

  - 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/20151019/b3045f1c/attachment.htm>

More information about the hotspot-gc-dev mailing list