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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
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 ([hidden email]) 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
Reply | Threaded
Open this post in threaded view
|

Re: 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
<[hidden email]> 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 ([hidden email]) 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
Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl
In reply to this post by Volker Simonis
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 ([hidden email]) 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

Reply | Threaded
Open this post in threaded view
|

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

Kim Barrett
In reply to this post by Volker Simonis
> On Feb 27, 2017, at 12:10 PM, Volker Simonis <[hidden email]> 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

looks good

Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
In reply to this post by Thomas Schatzl
Yes, please. That would be great!

Thanks,
Volker


On Mon, Mar 6, 2017 at 11:20 AM, Thomas Schatzl
<[hidden email]> 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 ([hidden email]) 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
>
Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
In reply to this post by Kim Barrett
Thanks Kim,
Volker


On Mon, Mar 6, 2017 at 6:30 PM, Kim Barrett <[hidden email]> wrote:

>> On Feb 27, 2017, at 12:10 PM, Volker Simonis <[hidden email]> 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
>
> looks good
>
Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl
In reply to this post by Volker Simonis
Hi Volker,

On Mon, 2017-03-06 at 19:14 +0100, Volker Simonis wrote:
> Yes, please. That would be great!

  on its way.

Gunter already has at least four contributions now, maybe you could
help him apply for authorship?

Thanks,
  Thomas