RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Derek White]

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

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

I cant' recall how to mark a bug as blocking another bug.
>
> 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/20151007/97eb543f/attachment.htm>