RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Bengt Rutisson]

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.

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

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


>>
>> In ConcurrentG1SampleThread::sample_young_list_rs_lengths() you 
>> copied the code that does:
>>
>>   if (g1p->adaptive_young_list_length()) {
>>
>> As far as I can tell this is always true, since the value for 
>> adaptive_young_list_length() stems from 
>> G1YoungGenSizer::_adaptive_size, which only seems to be set in the 
>> constructor:
>>
>> G1YoungGenSizer::G1YoungGenSizer() : _sizer_kind(SizerDefaults), 
>> _adaptive_size(true)
>>
>> Maybe this should be fixed separately, but it looks like it should be 
>> cleaned up.
> Ignored as requested.
>
> Thanks Bengt!
>
>  - Derek

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20151008/9faf901c/attachment.htm>