RFR(XS): 8175900: Assertion too strict in G1CollectedHeap::new_mutator_alloc_region

[Volker Simonis]

Yes, please. That would be great!

Thanks,
Volker


On Mon, Mar 6, 2017 at 11:20 AM, Thomas Schatzl
<thomas.schatzl at oracle.com> wrote:
> Hi,
>
> On Mon, 2017-02-27 at 18:10 +0100, Volker Simonis wrote:
>> Hi,
>>
>> can I please have a review and sponsor for the following tiny change:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8175900/
>> https://bugs.openjdk.java.net/browse/JDK-8175900
>>
>> Gunter Haug (gunter.haug at sap.com) provided the following bug report
>> and fix:
>>
>> The assertion (!force || g1_policy()->can_expand_young_list()) in
>> G1CollectedHeap::new_mutator_alloc_region appears to be too strict.
>> In
>> fact, new_mutator_alloc_region with force=true is called from
>> attempt_allocation_slow only under the condition of
>> g1_policy()->can_expand_young_list() indirectly, via this hierarchy:
>>
>> G1CollectedHeap::new_mutator_alloc_region(unsigned long, bool)
>>   MutatorAllocRegion::allocate_new_region(unsigned long, bool)
>>     G1AllocRegion::new_alloc_region_and_allocate(unsigned long, bool)
>>       G1AllocRegion::attempt_allocation_force(unsigned long, bool)
>>         G1CollectedHeap::attempt_allocation_slow(unsigned long,
>> unsigned char, unsigned int*, int*)
>>
>> However, this happens in a mutator thread while the
>> G1YoungRemSetSamplingThread is running concurrently and may call
>> revise_young_list_target_length_if_necessary(), which in turn calls
>> update_max_gc_locker_expansion() where _young_list_max_length may be
>> decreased and therefor g1_policy()->can_expand_young_list() is not
>> true anymore.
>>
>> This behavior is rare, we only observed it a few times in our nightly
>> tests over several years. However, by suspending the mutator thread
>> in attempt_allocation_slow, for a few seconds before calling
>> attempt_allocation_force, we were able to reproduce the behavior.
>>
>> We'd like to propose to remove the assertion.
>
> Okay, looks good.
>
>>
>> Also, _young_list_max_length and the other counters in
>> G1DefaultPolicy which are accessed concurrently should be declared
>> volatile. But that should be handled in an separate issue.
>
> Okay.
>
>
> I assume that you also need a sponsor for that change, with you
> (simonis) as second reviewer?
>
> Thanks,
>   Thomas
>