RFR: 8264649: runtime/InternalApi/ThreadCpuTimesDeadlock.java crash in fastdebug C2 with -XX:-UseTLAB

[Hui Shi-2]

Detailed analysis in https://bugs.openjdk.java.net/browse/JDK-8264649

Tier1/2 release /fastdebug passed

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

Commit messages:
 - 8264649: runtime/InternalApi/ThreadCpuTimesDeadlock.java crash in fastdebug C2 with -XX:-UseTLAB

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

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

[Tobias Hartmann-3]

On Sat, 3 Apr 2021 00:53:54 GMT, Hui Shi <[hidden email]> wrote:

> Please help review this fix. Detailed analysis in https://bugs.openjdk.java.net/browse/JDK-8264649
>
> Tier1/2 release /fastdebug passed

Changes requested by thartmann (Reviewer).

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

> 1479:   // Smash all inputs to 'old', isolating him completely
> 1480:   Node *temp = new Node(1);
> 1481:   temp->init_req(0,nn);     // Add a use to nn to prevent him from dying

Just wondering, why do we even need this? Without that code, `remove_dead_node(old)` would kill `nn` as well if it becomes dead.

test/hotspot/jtreg/runtime/InternalApi/ThreadCpuTimesDeadlock.java line 36:

> 34:  * @test
> 35:  * @bug 8264649
> 36:  * @summary OSR compiled metthod crash when UseTLAB is off

Typo `metthod`.

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

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

[Tobias Hartmann-3]

On Wed, 7 Apr 2021 12:09:05 GMT, Hui Shi <[hidden email]> wrote:

>> Please help review this fix. Detailed analysis in https://bugs.openjdk.java.net/browse/JDK-8264649
>>
>> Tier1/2 release /fastdebug passed
>
> Hui Shi has updated the pull request incrementally with one additional commit since the last revision:
>
>   fix typo in test description

Thanks for the details. Your fix looks reasonable to me.

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

Marked as reviewed by thartmann (Reviewer).

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

[Vladimir Kozlov-2]

On Wed, 7 Apr 2021 12:00:10 GMT, Hui Shi <[hidden email]> wrote:

>> src/hotspot/share/opto/phaseX.cpp line 1481:
>>
>>> 1479:   // Smash all inputs to 'old', isolating him completely
>>> 1480:   Node *temp = new Node(1);
>>> 1481:   temp->init_req(0,nn);     // Add a use to nn to prevent him from dying
>>
>> Just wondering, why do we even need this? Without that code, `remove_dead_node(old)` would kill `nn` as well if it becomes dead.
>
> Thanks for your review!
>
> Checking code history,  this code is quite old. From comments around, it doesn't want nn removed directly in PhaseIterGVN::subsume_node and leaves optimization to next GVN iteration.
>
> In my understanding, it might save callers to insert codes checking if "nn" is removed/dead at every subsume_node/replace_node callsite, simplifies implementation.
>
> 8153779a hotspot/src/share/vm/opto/phaseX.cpp (J. Duke                  2007-12-01 00:00:00 +0000 1479)   // Smash all inputs to 'old', isolating him completely
> 2a0815a5 hotspot/src/share/vm/opto/phaseX.cpp (Tobias Hartmann          2014-06-02 08:07:29 +0200 1480)   Node *temp = new Node(1);
> 8153779a hotspot/src/share/vm/opto/phaseX.cpp (J. Duke                  2007-12-01 00:00:00 +0000 1481)   temp->init_req(0,nn);     // Add a use to nn to prevent him from dying
> 8153779a hotspot/src/share/vm/opto/phaseX.cpp (J. Duke                  2007-12-01 00:00:00 +0000 1482)   remove_dead_node( old );
> 8153779a hotspot/src/share/vm/opto/phaseX.cpp (J. Duke                  2007-12-01 00:00:00 +0000 1483)   temp->del_req(0);         // Yank bogus edge

Note `nn` could be new node created during parsing which does not have users yet. that is why we need to keep it

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

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

[Vladimir Kozlov-2]

On Wed, 7 Apr 2021 12:09:05 GMT, Hui Shi <[hidden email]> wrote:

>> Please help review this fix. Detailed analysis in https://bugs.openjdk.java.net/browse/JDK-8264649
>>
>> Tier1/2 release /fastdebug passed
>
> Hui Shi has updated the pull request incrementally with one additional commit since the last revision:
>
>   fix typo in test description

I agree with change.

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

Marked as reviewed by kvn (Reviewer).

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

[Hui Shi-2]

On Fri, 9 Apr 2021 07:01:01 GMT, Tobias Hartmann <[hidden email]> wrote:

>> Hui Shi has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   fix typo in test description
>
> Thanks for the details. Your fix looks reasonable to me.

Thanks! @TobiHartmann @vnkozlov

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

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

[Hui Shi-2]

On Sat, 3 Apr 2021 00:53:54 GMT, Hui Shi <[hidden email]> wrote:

> Please help review this fix. Detailed analysis in https://bugs.openjdk.java.net/browse/JDK-8264649
>
> Tier1/2 release /fastdebug passed

This pull request has now been integrated.

Changeset: 42f4d706
Author:    Hui Shi <[hidden email]>
Committer: Jie Fu <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/42f4d706
Stats:     13 lines in 2 files changed: 12 ins; 0 del; 1 mod

8264649: runtime/InternalApi/ThreadCpuTimesDeadlock.java crash in fastdebug C2 with -XX:-UseTLAB

Reviewed-by: thartmann, kvn

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

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