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

[Mikael Gerdin]

Hi Derek,

On 2016-02-25 17:50, Derek White wrote:
> Ok, sounds good. reviewed.

Thanks for the review!

/Mikael

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