8279515: C1: No inlining through invokedynamic and invokestatic call sites when resolved class is not linked

[iwanowww]

JDK-8267806 broke inlining at invokedynamic and invokestatic call sites in some circumstances. Though the enhancement was intended to relax the inlining checks, it introduced one new check to ensure that resolved klass (REFC) is linked. It turned out the check is too strong: resolution of the symbolic class reference at a call site does not trigger class linkage.

For invokestatic consider the following example:

class A { static m() { ... } }
class B extends A {}

invokestatic "B"::"m" => A::m

Method resolution loads class B along the way, but neither initialises nor links it. Invocation does trigger class initialisation, but only for class A.

For invokedynamic it's a bit more complicated. Syntactically, there's no symbolic class reference at the call site, but CI assigns an artificial one (java.lang.invoke.MethodHandle, see ciBytecodeStream::get_declared_method_holder()). The problem is MethodHandle class doesn't have to be loaded by the class loader of the accessing class, so ciEnv::get_klass_by_name() can produce unloaded CI mirror (ciKlass).

For example, indy call site linkage of indified string concatenation (StringConcatFactory.makeConcatWithConstants()) doesn't get MethodHandle class recorded in the context class loader [1] while LambdaMetafactory.metafactory() introduces a class loader constraint for it [2] which reveals the class to the subsequent class lookup. ​

I decided to make ciBytecodeStream::get_declared_method_holder() predictable and return well-known VM class right away without doing the lookup.

Proposed fix relaxes the constraint on resolved class (REFC) and requires it to be loaded.

Also, while working on the fix, I spotted that the constraint for invokestatic is also too strong (requires resolved class to be initialized while the specification mandates resolved method holder to be initialized instead):

    if ((code == Bytecodes::_invokestatic && callee_holder->is_initialized()) || // invokestatic involves an initialization barrier on resolved klass

I fixed it as well (s/callee_holder/klass/).

Testing: hs-tier1 - hs-tier4

[1]

[0.160s][debug][class,resolve           ] Test1 java.lang.invoke.StringConcatFactory Test.java:3
[0.160s][info ][class,loader,constraints] adding new constraint for name: java/lang/invoke/MethodHandles$Lookup, loader[0]: 'app', loader[1]: 'bootstrap'
[0.160s][info ][class,loader,constraints] adding new constraint for name: java/lang/invoke/MethodType, loader[0]: 'app', loader[1]: 'bootstrap'
[0.160s][info ][class,loader,constraints] adding new constraint for name: java/lang/invoke/CallSite, loader[0]: 'app', loader[1]: 'bootstrap'

[2]

[0.130s][debug][class,resolve          ] Test java.lang.invoke.LambdaMetafactory Test.java:21
[0.130s][info ][class,loader,constraints] adding new constraint for name: java/lang/invoke/MethodHandles$Lookup, loader[0]: 'app', loader[1]: 'bootstrap'
[0.130s][info ][class,loader,constraints] adding new constraint for name: java/lang/invoke/MethodType, loader[0]: 'app', loader[1]: 'bootstrap'
[0.130s][info ][class,loader,constraints] adding new constraint for name: java/lang/invoke/MethodHandle, loader[0]: 'app', loader[1]: 'bootstrap'
[0.130s][info ][class,loader,constraints] adding new constraint for name: java/lang/invoke/CallSite, loader[0]: 'app', loader[1]: 'bootstrap'

---

Progress

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

Issue

• JDK-8279515: C1: No inlining through invokedynamic and invokestatic call sites when resolved class is not linked

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Dean Long (@dean-long - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 80

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

Using diff file

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

[bridgekeeper]

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

@iwanowww 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

• 00: Full (91402f13)

[vnkozlov]

Seems reasonable.

[openjdk]

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

8279515: C1: No inlining through invokedynamic and invokestatic call sites when resolved class is not linked

Reviewed-by: kvn, dlong

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

• 564c8c6: 8279529: ProblemList java/nio/channels/DatagramChannel/ManySourcesAndTargets.java on macosx-aarch64
• 590fa9d: 8278612: [macos] test/jdk/java/awt/dnd/RemoveDropTargetCrashTest crashes with VoiceOver on macOS
• 5cd9515: 8279525: ProblemList java/awt/GraphicsDevice/CheckDisplayModes.java on macosx-aarch64

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.

[dean-long]

Solution makes sense. I had my doubts about the get_declared_method_holder() behavior.

[iwanowww]

Thanks for the review, Vladimir and Dean.

/integrate

[openjdk]

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

• 928e347: 8279032: compiler/loopopts/TestSkeletonPredicateNegation.java times out with -XX:TieredStopAtLevel < 4
• 354c904: 8273452: DocTrees.getDocCommentTree should be specified as idempotent
• 06b4d49: 8278344: sun/security/pkcs12/KeytoolOpensslInteropTest.java test fails because of different openssl output
• 8f969a1: 8278930: javac tries to compile a file twice via PackageElement.getEnclosedElements
• 642ab34: 8278373: JavacTrees.searchMethod finds incorrect match
• d65c665: 8279527: Dereferencing segments backed by different scopes leads to pollution
• 967ef0c: 8278020: ~13% variation in Renaissance-Scrabble
• 7c792f2: 8279333: Some JFR tests do not accept 'GCLocker Initiated GC' as a valid GC Cause
• 564c8c6: 8279529: ProblemList java/nio/channels/DatagramChannel/ManySourcesAndTargets.java on macosx-aarch64
• 590fa9d: 8278612: [macos] test/jdk/java/awt/dnd/RemoveDropTargetCrashTest crashes with VoiceOver on macOS
• ... and 1 more: https://git.openjdk.java.net/jdk18/compare/9d43d25da8bcfff425a795dcc230914a384a5c82...master

Your commit was automatically rebased without conflicts.

[openjdk]

@iwanowww Pushed as commit ad34f03.

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