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

[Mikael Gerdin]

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