RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Bengt Rutisson]

Hi again,

A short update on my comment about adaptive size inlined below.

On 2015-10-07 10:02, 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.
>
> 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.
>
> 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*.
>
> You write "and also fixes a P4 bug". Which bug is that?
>
>
> 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.

Scratch that. I missed that the G1YoungGenSizer constructor resets the 
_adaptive_size value after it has been initialized in the initialization 
list. So, the value can actually be false.

Sorry about the noise.

Bengt

>
>
> Thanks,
> Bengt
>
>>
>> Testing: jprt
>>
>> Perf testing: in progress...
>>
>>
>> Thanks!
>>
>>  - Derek
>>
>>
>>
>

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