RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Bengt Rutisson]

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.


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