8350086: Inline hot Method accessors for faster task selection

[shipilev]

These methods show up prominently on Leyden profiles, as compilation policy asks these properties for methods very often during compile task selection:

• Method::invocation_count
• Method::backedge_count
• Method::highest_comp_level

We can move the definitions for these methods to method.inline.hpp to make them eligible for better inlining.

interpreter_invocation_count() method is a bit weird, looks like a leftover from JDK-8251462. Removing it would prompt more cleanups and renamings in ciMethod, so I would leave it for future enhancement.

Additional testing:

• Spot-checked Leyden profiles, methods are now fully inlined into hot CompilerBroker methods
• Ad-hoc Leyden benchmarks show minor improvements (< 1%) for time spent in compiler threads

---

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-8350086: Inline hot Method accessors for faster task selection (Enhancement - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Coleen Phillimore (@coleenp - Reviewer)
• Andrew Haley (@theRealAph - Reviewer)
• Vladimir Ivanov (@iwanowww - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23634

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back shade! 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]

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

8350086: Inline hot Method accessors for faster task selection

Reviewed-by: kvn, coleenp, aph, vlivanov

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

• 2192723: 8350162: ProblemList compiler/tiered/Level2RecompilationTest.java
• 7d11418: 8350147: Replace example in KEM class with the one from JEP 452
• 5cf1132: 8350098: jpackage test lib erroneously will run methods without @test annotation as tests
• 6234536: 8349915: CTW: Lots of level 3 compiles are done at level 2 after JDK-8348570
• b6443f6: 8348347: Cleanup JavaThread subclass support in SA
• ba6c965: 8348595: GenShen: Fix generational free-memory no-progress check
• 3832240: 8348594: Shenandoah: Do not penalize for degeneration when not the fault of triggering heuristic
• 2a90b90: 8346117: Add test annotation
• 0414dce: 8349812: (fs) Files.newByteChannel with empty path name and CREATE_NEW throws unexpected exception
• 9ea81d9: 8349351: Combine Screen Inset Tests into a Single File

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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]

@shipilev 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.

[mlbridge]

Webrevs

• 02: Full - Incremental (24be8e68)
• 01: Full - Incremental (8f6d4fab)
• 00: Full (845dc7d5)

[vnkozlov]

Okay.

[coleenp]

This seems fine but at one point we were talking about moving what looks like the duplicated InvocationCounters from MethodCounters and use MDO instead. I think this looks like it could be something to clean up.

[theRealAph]

That's nice. It looks to me like those methods would only be a few instructions long, so the overhead of a subroutine would be disproportionate, and the cost of inlining small. Win-win.

[iwanowww]

The patch looks well-justified to me, but it feels like the focus is on a symptom and not the root cause.

These methods show up prominently on Leyden profiles, as compilation policy asks these properties for methods very often during compile task selection

That's the consequence of poor scaling properties CompilationPolicy::select_task() demonstrates. Each CompileQueue::get() call involves a linear pass over the whole compile queue (implemented as linked list) recomputing event rate each time. The longer the queue, the more time it takes to select next task to compile. And Leyden greatly exarcebates the problem by aggressively submitting compilation tasks based on training data.

FTR heavy lock contention on MethodCompileQueue_lock was another symptom of the very same problem. Proper fix would be to reimplement how compilation task prioritization is implemented.

[iwanowww]

This seems fine but at one point we were talking about moving what looks like the duplicated InvocationCounters from MethodCounters and use MDO instead. I think this looks like it could be something to clean up.

There's definitely some duplication between MethodCounter and MDO, but those two serve different purposes at runtime when it comes to profiling (facilitate different profiling modes). There are ways to merge them, but it may have far-reaching consequences for the implementation (e.g., fast MDO presence check is used to guard profiling logic in interpreter). Not clear to me whether it'll worth the effort.

[shipilev]

Thank you for reviews!

Yes, the core of the problem is potentially quadratic behavior in task selection, like Vladimir describes. We had this problem in Leyden for SCC tasks: openjdk/leyden#17 -- so it does not apply to SC loaded methods anymore. I agree it would be great to resolve the task selection problem at its core; unfortunately, my crude attempts at doing so failed, because tiered policy is quite fiddly. It does not mean we would not try again, it just means it would take a bit more time. Meanwhile, we can address little inefficiencies without solving the core issue.

To that extent, I would spin this more positively: now that Leyden is able to shift away the bulk of C2 compilations away, the little inefficiencies in normal compilation paths show up in those runs. The inefficiency is also in mainline, but it would be obscured by the heavy compilations that would follow the task selection.

So I think these kind of inlining improvements stand well on their own, and are still worth doing.

[shipilev]

/integrate

[openjdk]

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

• 5e9d72e: 8350094: Linux gcc 13.2.0 build fails when ubsan is enabled
• 2192723: 8350162: ProblemList compiler/tiered/Level2RecompilationTest.java
• 7d11418: 8350147: Replace example in KEM class with the one from JEP 452
• 5cf1132: 8350098: jpackage test lib erroneously will run methods without @test annotation as tests
• 6234536: 8349915: CTW: Lots of level 3 compiles are done at level 2 after JDK-8348570
• b6443f6: 8348347: Cleanup JavaThread subclass support in SA
• ba6c965: 8348595: GenShen: Fix generational free-memory no-progress check
• 3832240: 8348594: Shenandoah: Do not penalize for degeneration when not the fault of triggering heuristic
• 2a90b90: 8346117: Add test annotation
• 0414dce: 8349812: (fs) Files.newByteChannel with empty path name and CREATE_NEW throws unexpected exception
• ... and 1 more: https://git.openjdk.org/jdk/compare/742e735d7f6c4ee9ca5a4d290c59d7d6ec1f7635...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit b1b4828.

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