RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Derek White]

Hi Per,

On 10/9/15 4:56 AM, Per Liden wrote:
> Hi Derek,
>
>> On 09 Oct 2015, at 00:29, Derek White <derek.white at oracle.com 
>> <mailto:derek.white at oracle.com>> wrote:
>>
>> Another call for review:
>>
>> 2nd webrev:
>> http://cr.openjdk.java.net/~drwhite/8138920/webrev.02/ 
>> <http://cr.openjdk.java.net/%7Edrwhite/8138920/webrev.02/>
>
> Breaking out the sampling thread from refinement thread seems like a 
> really good idea. Thanks for addressing that.
>
> However, I have a problem with the new ConcurrentServiceThread, which 
> I don’t think we need. I don’t see a problem with moving some of the 
> common things to ConcurrentGCThread. I think you’re on the right 
> track, but too much thread specific logic (more specifically _monitor 
> and _notify_all) has leaked up into the common layer. You probably 
> just want a callback to the thread and let the thread itself figure 
> out what it needs to do to get stopped. That way I think you can have 
> the same model for all threads here, including the string dedup and 
> cms thread.
Yes, I was thinking that might work out. _notify_all was a hack. Even 
worse I suspect that it's not necessary (notify would have been fine), 
but I couldn't prove it.
>
> To avoid stalling what this change is really about (the sampling 
> thread fix), I'd suggest that we leave the ConcurrentServiceThread out 
> of this patch, let the sampling thread inherit form ConcurrentGCThread 
> and we can do a cleanup of the ConcurrentGCThread as a separate change 
> later.

OK, sounds good to me.
>
> cheers
> /Per

Thanks!

  - Derek
>
>>
>> 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.
>>
>>>>> 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.
>>>
>>>>> 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 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/f2169f67/attachment.htm>