8273712: C2: Add mechanism for rejecting inlining of low frequency call sites and deprecate MinInliningThreshold. #6046

veresov · 2021-10-20T17:29:41Z

Currently the inlining heuristic uses absolute method invocation count to reject methods that are rarely executed (see MinInliningThreshold and its uses).
This presents two problems:

Method can be rarely used in a particular caller, yet if its total execution count is high it may be still inlined.
The use of absolute counts is inherently problematic with the current compilation policy (adaptive threshold and background compilation). It leads to instabilities of inlining decisions.

The proposed solution is to consider call site execution ratio in order to reject callees that are rarely executed. Set the old cutoff parameter (MinInliningThreshold) to 0 to essentially disable it and later deprecate it.

Setting the introduced MinInlineFrequencyRatio = 0.0085 produces the following notable improvements:
Renaissance-Dotty 1.23%
Renaissance-Mnemonics 3.88%
Renaissance-NaiveBayes 9.23%
Renaissance-ScalaKmeans 1.36%
SPECjvm2008-Derby 3.16%

There are of course some regressions but those are few and on the order of 1.5%

This PR will require a CSR before it can be pushed. I'll file a CSR after this is reviewed.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8273712: C2: Add mechanism for rejecting inlining of low frequency call sites and deprecate MinInliningThreshold.

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Rickard Bäckman (@rickard - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6046/head:pull/6046
$ git checkout pull/6046

Update a local copy of the PR:
$ git checkout pull/6046
$ git pull https://git.openjdk.java.net/jdk pull/6046/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6046

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6046.diff

bridgekeeper · 2021-10-20T17:31:25Z

👋 Welcome back iveresov! 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 · 2021-10-20T17:33:47Z

@veresov The following label will be automatically applied to this pull request:

hotspot

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.

veresov · 2021-10-20T17:34:32Z

/label add hotspot-compiler

veresov · 2021-10-20T17:34:42Z

/label remove hotspot

openjdk · 2021-10-20T17:35:59Z

@veresov
The hotspot-compiler label was successfully added.

openjdk · 2021-10-20T17:38:54Z

@veresov
The hotspot label was successfully removed.

mlbridge · 2021-10-20T17:42:17Z

Webrevs

00: Full (9c1c5f88)

vnkozlov

Looks fine but I don't see CSR filed for MinInliningThreshold which is product flag.

openjdk · 2021-10-20T22:26:35Z

@veresov 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:

8273712: C2: Add mechanism for rejecting inlining of low frequency call sites and deprecate MinInliningThreshold.

Reviewed-by: kvn, rbackman

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 63 new commits pushed to the master branch:

cea3f01: 8275666: serviceability/jvmti/GetObjectSizeClass.java shouldn't have vm.flagless
d1e3ca4: 8233558: [TESTBUG] WindowOwnedByEmbeddedFrameTest.java fails on macos
913f928: 8273507: Convert test/jdk/java/nio/channels/Channels/TransferTo.java to TestNG test
0021a2f: 8275449: Add linux-aarch64-zero build profile
46b5bfb: 8233648: [TESTBUG] DefaultMenuBarTest.java failing on macos
bbc6061: 8272614: Unused parameters in MethodHandleNatives linking methods
7e28bdd: 8275055: Improve HeapRegionRemSet::split_card()
35e5bb5: 8269336: Malformed jdk.serialFilter incorrectly handled
043cde2: 8275319: java.net.NetworkInterface throws java.lang.Error instead of SocketException
a91a0a5: 8233724: Remove -Wc++14-compat warning suppression in operator_new.cpp
... and 53 more: https://git.openjdk.java.net/jdk/compare/72a976ef05fc2c62657920a560a0abc60b27c852...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.

rickard

Looks good.

veresov · 2021-10-20T22:47:56Z

Looks fine but I don't see CSR filed for MinInliningThreshold which is product flag.

I'm going to file it now.

veresov · 2021-10-20T23:03:34Z

I've filed a CSR (https://bugs.openjdk.java.net/browse/JDK-8275676). Could you guys please yourselves as reviewers to it? Thanks!

veresov · 2021-10-25T17:02:21Z

/integrate

openjdk · 2021-10-25T17:04:10Z

Going to push as commit 89671aa.
Since your change was applied there have been 106 commits pushed to the master branch:

3221a14: 8273678: TableAccessibility and TableRowAccessibility miss autorelease
7cf68b1: 8202932: java/awt/Component/NativeInLightShow/NativeInLightShow.java fails
f610ef0: 8196440: Regression automated Test 'java/awt/TrayIcon/PopupMenuLeakTest/PopupMenuLeakTest.java' fails
f143d2a: 8268595: java/io/Serializable/serialFilter/GlobalFilterTest.java#id1 failed in timeout
7f94302: 8275511: G1: Rename needs_remset_update to remset_is_tracked in G1HeapRegionAttr
0bcc174: 8275717: Reimplement STATIC_ASSERT to use static_assert
f623298: 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key
1da5e6b: 8275104: IR framework does not handle client VM builds correctly
5dab76b: 8275381: G1: refactor 2 constructors of G1CardSetConfiguration
5bbe4ca: 8275293: A change done with JDK-8268764 mismatches the java.rmi.server.ObjID.hashCode spec
... and 96 more: https://git.openjdk.java.net/jdk/compare/72a976ef05fc2c62657920a560a0abc60b27c852...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-25T17:04:29Z

@veresov Pushed as commit 89671aa.

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

veresov added 2 commits October 18, 2021 10:07

Make low frequency inling cutoff relative

7875914

Remove old option from a test

9c1c5f8

openjdk bot added the rfr Pull request is ready for review label Oct 20, 2021

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 20, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 20, 2021

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Oct 20, 2021

vnkozlov approved these changes Oct 20, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 20, 2021

rickard approved these changes Oct 20, 2021

View reviewed changes

openjdk bot closed this Oct 25, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8273712: C2: Add mechanism for rejecting inlining of low frequency call sites and deprecate MinInliningThreshold. #6046

8273712: C2: Add mechanism for rejecting inlining of low frequency call sites and deprecate MinInliningThreshold. #6046

Uh oh!

veresov commented Oct 20, 2021 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Oct 20, 2021

Uh oh!

openjdk bot commented Oct 20, 2021

Uh oh!

veresov commented Oct 20, 2021

Uh oh!

veresov commented Oct 20, 2021

Uh oh!

openjdk bot commented Oct 20, 2021

Uh oh!

openjdk bot commented Oct 20, 2021

Uh oh!

mlbridge bot commented Oct 20, 2021

Uh oh!

vnkozlov left a comment

Uh oh!

openjdk bot commented Oct 20, 2021 •

edited

Loading

Uh oh!

rickard left a comment

Uh oh!

veresov commented Oct 20, 2021

Uh oh!

veresov commented Oct 20, 2021

Uh oh!

veresov commented Oct 25, 2021

Uh oh!

openjdk bot commented Oct 25, 2021

Uh oh!

openjdk bot commented Oct 25, 2021

Uh oh!

Uh oh!

8273712: C2: Add mechanism for rejecting inlining of low frequency call sites and deprecate MinInliningThreshold. #6046

8273712: C2: Add mechanism for rejecting inlining of low frequency call sites and deprecate MinInliningThreshold. #6046

Uh oh!

Conversation

veresov commented Oct 20, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 20, 2021

Uh oh!

openjdk bot commented Oct 20, 2021

Uh oh!

veresov commented Oct 20, 2021

Uh oh!

veresov commented Oct 20, 2021

Uh oh!

openjdk bot commented Oct 20, 2021

Uh oh!

openjdk bot commented Oct 20, 2021

Uh oh!

mlbridge bot commented Oct 20, 2021

Webrevs

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickard left a comment

Choose a reason for hiding this comment

Uh oh!

veresov commented Oct 20, 2021

Uh oh!

veresov commented Oct 20, 2021

Uh oh!

veresov commented Oct 25, 2021

Uh oh!

openjdk bot commented Oct 25, 2021

Uh oh!

openjdk bot commented Oct 25, 2021

Uh oh!

Uh oh!

veresov commented Oct 20, 2021 •

edited by openjdk bot

Loading

openjdk bot commented Oct 20, 2021 •

edited

Loading