RFR: 8260653: Unreachable nodes keep speculative types alive

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

RFR: 8260653: Unreachable nodes keep speculative types alive

Nils Eliasson-2
8260653: Unreachable nodes keep speculative types alive

-------------

Commit messages:
 - Remove useless nodes

Changes: https://git.openjdk.java.net/jdk/pull/2606/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260653
  Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2606.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2606/head:pull/2606

PR: https://git.openjdk.java.net/jdk/pull/2606
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260653: Unreachable nodes keep speculative types alive

Nils Eliasson-2
On Wed, 17 Feb 2021 10:27:08 GMT, Nils Eliasson <[hidden email]> wrote:

> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
>
> This problem will not cause crashes in production - it is only a sanity test.
>
> I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left.
>
> When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive.
>
> A big thank you to Dean Long how created the reproducer for this bug.
>
> Please review.

One reasonable alternative to adding one more pass of PhaseRemoveUseless is filtering out all non-reachable nodes from NodeHash::check_no_speculative_types. The unreachable nodes only live a short while before being removed in the renumbering phase.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2606
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v2]

Nils Eliasson-2
In reply to this post by Nils Eliasson-2
> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
>
> This problem will not cause crashes in production - it is only a sanity test.
>
> I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left.
>
> When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive.
>
> A big thank you to Dean Long how created the reproducer for this bug.
>
> Please review.

Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision:

  Check if node is live before assert

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2606/files
  - new: https://git.openjdk.java.net/jdk/pull/2606/files/5f279179..82ab1066

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=00-01

  Stats: 10 lines in 2 files changed: 6 ins; 3 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2606.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2606/head:pull/2606

PR: https://git.openjdk.java.net/jdk/pull/2606
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v2]

Vladimir Ivanov-2
On Wed, 17 Feb 2021 14:35:59 GMT, Nils Eliasson <[hidden email]> wrote:

>> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
>>
>> This problem will not cause crashes in production - it is only a sanity test.
>>
>> I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left.
>>
>> When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive.
>>
>> A big thank you to Dean Long how created the reproducer for this bug.
>>
>> Please review.
>
> Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision:
>
>   Check if node is live before assert

Looks good.

src/hotspot/share/opto/phaseX.cpp line 341:

> 339:        n != sentinel_node &&
> 340:        n->is_Type() &&
> 341:        n->outcnt() > 0 &&

`live_nodes.member(n)` makes ` n->outcnt() > 0` redundant, doesn't it?

-------------

Marked as reviewed by vlivanov (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2606
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v3]

Nils Eliasson-2
In reply to this post by Nils Eliasson-2
> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
>
> This problem will not cause crashes in production - it is only a sanity test.
>
> I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left.
>
> When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive.
>
> A big thank you to Dean Long how created the reproducer for this bug.
>
> Please review.

Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision:

  Remove outcnt check

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2606/files
  - new: https://git.openjdk.java.net/jdk/pull/2606/files/82ab1066..ac23dad8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2606.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2606/head:pull/2606

PR: https://git.openjdk.java.net/jdk/pull/2606
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v2]

Nils Eliasson-2
In reply to this post by Vladimir Ivanov-2
On Wed, 17 Feb 2021 16:34:43 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Check if node is live before assert
>
> src/hotspot/share/opto/phaseX.cpp line 341:
>
>> 339:        n != sentinel_node &&
>> 340:        n->is_Type() &&
>> 341:        n->outcnt() > 0 &&
>
> `live_nodes.member(n)` makes ` n->outcnt() > 0` redundant, doesn't it?

Yes it does. I've updated the patch.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2606
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v3]

Tobias Hartmann-3
In reply to this post by Nils Eliasson-2
On Wed, 17 Feb 2021 18:16:02 GMT, Nils Eliasson <[hidden email]> wrote:

>> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
>>
>> This problem will not cause crashes in production - it is only a sanity test.
>>
>> I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left.
>>
>> When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive.
>>
>> A big thank you to Dean Long how created the reproducer for this bug.
>>
>> Please review.
>
> Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision:
>
>   Remove outcnt check

Otherwise looks good.

src/hotspot/share/opto/phaseX.cpp line 338:

> 336:   for (uint i = 0; i < max; ++i) {
> 337:     Node *n = at(i);
> 338:     if(n != NULL &&

Missing whitespace between `if` and `(`

-------------

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2606
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v4]

Nils Eliasson-2
In reply to this post by Nils Eliasson-2
> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
>
> This problem will not cause crashes in production - it is only a sanity test.
>
> I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left.
>
> When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive.
>
> A big thank you to Dean Long how created the reproducer for this bug.
>
> Please review.

Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision:

  Add missing ws

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2606/files
  - new: https://git.openjdk.java.net/jdk/pull/2606/files/ac23dad8..ae048cbc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2606&range=02-03

  Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2606.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2606/head:pull/2606

PR: https://git.openjdk.java.net/jdk/pull/2606
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8260653: Unreachable nodes keep speculative types alive [v3]

Nils Eliasson-2
In reply to this post by Tobias Hartmann-3
On Thu, 18 Feb 2021 08:10:08 GMT, Tobias Hartmann <[hidden email]> wrote:

>> Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Remove outcnt check
>
> Otherwise looks good.

Thanks for the reviews Vladimir and Tobias!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2606
Reply | Threaded
Open this post in threaded view
|

Integrated: 8260653: Unreachable nodes keep speculative types alive

Nils Eliasson-2
In reply to this post by Nils Eliasson-2
On Wed, 17 Feb 2021 10:27:08 GMT, Nils Eliasson <[hidden email]> wrote:

> The RunThese test fails because after first igvn.optimize() (directly after parsing) there are unreachable nodes cycles left. Later when remove_speculative_types() is called - only reachable nodes will have their speculative type removed. At the end of remove_speculative_types() there is an assert that all speculative types have been removed that will fail.
>
> This problem will not cause crashes in production - it is only a sanity test.
>
> I suggest adding a call to PhaseRemoveUseless before remove_speculative_types. This will cost a few extra cycles but it is the only way we can guarantee that no unreachable nodes are left.
>
> When debugging this I experimented with adding a call to verify_graph_edges to check for dead code at the same spot. This triggers failures in a lot of test. The conclusion is that it is very common that we have dead node cycles - but they very rarely keep speculative types alive.
>
> A big thank you to Dean Long how created the reproducer for this bug.
>
> Please review.

This pull request has now been integrated.

Changeset: 3a21e1df
Author:    Nils Eliasson <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/3a21e1df
Stats:     6 lines in 1 file changed: 5 ins; 0 del; 1 mod

8260653: Unreachable nodes keep speculative types alive

Reviewed-by: vlivanov, thartmann

-------------

PR: https://git.openjdk.java.net/jdk/pull/2606