RFR (M) JDK-8150390: Move rs length sampling data to the sampling thread

[Derek White]

Ok, sounds good. reviewed.

On 2/25/16 11:00 AM, Mikael Gerdin wrote:
> Hi Derek,
>
> On 2016-02-25 16:48, Derek White wrote:
>> On 2/25/16 9:46 AM, Mikael Gerdin wrote:
>>> Hi all,
>>>
>>> The G1 Young RemSet Sampling Thread iterates over the young regions to
>>> estimate the remembered set sizes of the young gen in order to tweak
>>> the young gen size between gcs.
>>>
>>> All the iteration and the state related to this has been stored in the
>>> YoungList class for some strange reason. The state is actually only
>>> ever needed in
>>> G1YoungRemSetSamplingThread::sample_young_list_rs_lengths()
>>> so I've moved all of it there.
>>> I also modified revise_young_list_target_length_if_necessary to
>>> receive the new rs length sample as a parameter instead of picking it
>>> up from the YoungList class.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8150390
>>> Webrev: http://cr.openjdk.java.net/~mgerdin/8150390/webrev.0
>>> Testing: Local GC test suite and JPRT
>>>
>>> Thanks
>>> /Mikael
>> Much cleaner!
>
> Thanks :)
>
>>
>> Some questions:
>>   - In G1YoungRemSetSamplingThread::sample_young_list_rs_lengths(), if
>> we yield we now return directly and don't call
>> revise_young_list_target_length_if_necessary(), but before we would call
>> it, even though data might be incomplete. Is this the right choice? (It
>> sounds reasonable to me, but I can't prove it).
>
> In fact, if you look closely at the old code it only set 
> _last_sampled_rs_lengths when iteration got to the end of the young list.
> If the iteration was aborted then 
> revise_young_list_target_length_if_necessary() would read the 
> previously set value of _last_sampled_rs_lengths and in that case I 
> would assume that _rs_lengths_prediction would be equal to 
> _last_sampled_rs_lengths and the code would do nothing.
>
>
>>
>>   - That code around the yield is very conservative. Maybe a GC
>> occurred, or maybe not. Can't we tell if it did or not, and only bail
>> out then? Yes, this is a pre-existing issue :-)
>
> I thought about attempting to look at the total collections counter 
> but reading that involves taking the Heap_lock, which could cause us 
> to block for a safepoint so I think trying to figure that out is best 
> left for a follow up enhancement.
>
> /Mikael
>
>>
>>   - Derek
>>
>>
>>
>>