RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Per Liden]

Hi Derek,

> On 09 Oct 2015, at 00:29, Derek White <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/~drwhite/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.

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.

cheers
/Per

> 
> 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/ <http://cr.openjdk.java.net/%7Edrwhite/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/a4d5833b/attachment.htm>