8353041: NeverBranchNode causes incorrect block frequency calculation

[dean-long]

This fixes a quality of implementation issue for infinite loops using a NeverBranch node. We need Block::succ_prob() to return 1.0f for the 100% successful back-edge so that block frequencies are computed correctly. I also fixed Block_Stack::most_frequent_successor() to choose the correct successor. I verified that this corrects the huge frequency ratio that was detected and clamped by JDK-8346888.

Currently this bug is labeled noreg-hard with no new regression test, as it's not obvious how to write such as test.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8353041: NeverBranchNode causes incorrect block frequency calculation (Bug - P3)

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Roberto Castañeda Lozano (@robcasloz - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24390/head:pull/24390
$ git checkout pull/24390

Update a local copy of the PR:
$ git checkout pull/24390
$ git pull https://git.openjdk.org/jdk.git pull/24390/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24390

View PR using the GUI difftool:
$ git pr show -t 24390

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24390.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back dlong! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@dean-long This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8353041: NeverBranchNode causes incorrect block frequency calculation

Reviewed-by: thartmann, rcastanedalo

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 183 new commits pushed to the master branch:

• 6df34c3: 8353174: Clean up thread register handling after 32-bit x86 removal
• 0f70aae: 8353841: [jittester] Fix JITTester build after asm removal
• 3340e13: 8352994: ZGC: Fix regression introduced in JDK-8350572
• ... and 180 more: https://git.openjdk.org/jdk/compare/3d2c3cd40ebce901d09a2479c267342e04e6f79c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@dean-long The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (2c9c3648)
• 00: Full (ac25f7ac)

[TobiHartmann]

Looks good to me.

[robcasloz]

Currently this bug is labeled noreg-hard with no new regression test, as it's not obvious how to write such as test.

The only idea I can think of would be matching and asserting on the output of -XX:+PrintCFGBlockFreq, e.g. for the first test in compiler/loopopts/TestPhaseCFGNeverBranchToGotoMain.java I get the line

   Loop: 1  trip_count: 1000000 freq:      0

before this fix, and the line

   Loop: 1  trip_count: 1000000 freq: 900000

after the fix. You could assert that you expect a freq greater than 0 for every encountered loop, or something similar. But I guess such a test would be quite fragile.

[dean-long]

@robcasloz , that's a good suggestion to use -XX:+PrintCFGBlockFreq to check the result, but I agree, writing a test based on it does seem fragile. Given that this code rarely changes (the bug has existed for 15+ years before it was noticed), I would expect the cost of the test (maintenance to prevent false positives) would exceed its value in finding actual regressions in this code. Are reviewers OK with pushing this as-is w/o a regression test?

[TobiHartmann]

Are reviewers OK with pushing this as-is w/o a regression test?

Fine with me.

[robcasloz]

@robcasloz , that's a good suggestion to use -XX:+PrintCFGBlockFreq to check the result, but I agree, writing a test based on it does seem fragile. Given that this code rarely changes (the bug has existed for 15+ years before it was noticed), I would expect the cost of the test (maintenance to prevent false positives) would exceed its value in finding actual regressions in this code. Are reviewers OK with pushing this as-is w/o a regression test?

Sure, I agree with your cost/benefit analysis. If we ever find more bugs in the future due to unexpected successor order for NeverBranch or other node classes, we should consider enforcing a canonical successor order during or right after control-flow graph construction.

[dean-long]

Thanks Roberto and Tobias.

[dean-long]

/integrate

[openjdk]

Going to push as commit 776e1cf.
Since your change was applied there have been 202 commits pushed to the master branch:

• cc546e7: 8353549: Open source events tests batch2
• faacbd9: 8353938: hotspot/jtreg/serviceability/dcmd/jvmti/LoadAgentDcmdTest.java fails on static JDK
• 6352ee1: 8349007: The jtreg test ResolvedMethodTableHash takes excessive time
• ... and 199 more: https://git.openjdk.org/jdk/compare/3d2c3cd40ebce901d09a2479c267342e04e6f79c...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dean-long Pushed as commit 776e1cf.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.