RFR: 8138920: Refactor the sampling thread from ConcurrentG1RefineThread

[Derek White]

Thanks Thomas,

On 10/21/15 4:51 AM, Thomas Schatzl wrote:
> Hi,h
>
> On Tue, 2015-10-20 at 15:56 -0400, Derek White wrote:
>> 4th Webrev for review please:
>>
>> This version includes the cleanups suggested by Per.
>>
>> RFE: JDK-8138920 Refactor the sampling thread from
>> ConcurrentG1RefineThread
>>
>> Webrev: http://cr.openjdk.java.net/~drwhite/8138920/webrev.04/
>>
>> jprt: waiting for results (which might take a long time given current
>> jprt status).
>>
>> Thanks for looking!
> Looks good overall. Some minor comments:
>
> - I am okay with deferring the move of the sample thread to another CR.
> Is there a CR for that already?
JDK-8140255 Move the management of G1YoungRemSetSamplingThread from 
ConcurrentG1Refine
<https://bugs.openjdk.java.net/browse/JDK-8140255>
> - the comment for ConcurrentG1Refine::sampling_thread() might need some
> elaboration.
I agree. :-) If you have any suggestions...
> - also add some description of the purpose of the new
> G1YoungListSampleThread class.
You are assuming that I understand what this code is really doing (and 
why) :-)

OK, in order to understand this better I really needed a definition of 
"MMU". Surprisingly I could find no definition of MMU in the source 
code. Although that seems to be a more major deficiency in our 
documentation, I'd like to finish 8138920 someday. See "JDK-8140251 
<https://bugs.openjdk.java.net/browse/JDK-8140251> Define the G1 term 
MMU somewhere in the source code".

Still looking into how this all fits together. I ask myself - How is 
this related to 
ConcurrentMarkThread::handle_adaptive_young_list_length()? Does that 
really have to do with marking? Where are other parts of this 
functionality hidden? What is that beautiful house? Where does that 
highway go to? Am I right?...Am I wrong? My God!...What have I done?!

Same as it ever was... Still looking...
> I am okay with the name, it could be more
> specific like G1YoungRemSetSamplingThread. I will let you decide whether
> to keep it.
OK, changed.
> - please rename
> ConcurrentMarkThread::handle_adaptive_young_list_length() to something
> more meaningful. The method delays the current thread so that the next
> expected pauses observe the MMU. (I think it is really good to extract
> this into a method!)
> So something like observe_mmu() or delay_thread_to_keep_mmu() would be
> imo more appropriate. These are just suggestions I came up within a few
> seconds though...
Those sound better, but still looking...
> - please also add some description of what these new methods do if not
> really obvious. E.g. that handle_adaptive_young_list_length() method
> does not qualify for obvious :)
Lots of non-obvious here. I found it pretty confusing that the variable 
"adaptive_young_list_length" is not actually a length, but a boolean!
>   I saw that the change fixes existing
> comments, but they are few and far between.
>
> Thanks for handling this issue,
>    Thomas

OK, thanks Thomas. I'll get a new webrev out after some more research.

  - Derek

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