RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Bengt Rutisson]

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/20151009/08ed586f/attachment.htm>