RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Derek White]

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/

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/20151016/93427036/attachment.htm>