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

[Volker Simonis]

Ping!

Can somebody please take a look at this change?

Thanks,
Volker

On Mon, Feb 27, 2017 at 6:10 PM, Volker Simonis
<volker.simonis at gmail.com> 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.
>
> 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.
>
> Thank you and best regards,
> Volker