8285394: Compiler blackholes can be eliminated due to stale ciMethod::intrinsic_id()

[shipilev]

This is seen in some tests: if blackhole method is deemed hot for inlining, then at least C2 would inline it without looking back at its intrinsic status. Which silently breaks blackholes.

The cause is that there are two places where intrinsic ID is recorded. Current blackhole code only writes down blackhole intrinsic ID in Method::intrinsic_id(), but we should also set it in ciMethod::intrinsic_id(), which is used from C2 inlining code. ciMethod is normally populated from Method::intrinsic_id(), but it happens too early, before setting up blackhole intrinsic.

Additional testing:

• Linux x86_64 {fastdebug,release}, wew test fails before the patch, passes with it
• Linux x86_64 {fastdebug,release} compiler/blackhole
• Linux x86_64 fastdebug, sanity microbenchmark corpus run with the patch

---

Progress

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

Issue

• JDK-8285394: Compiler blackholes can be eliminated due to stale ciMethod::intrinsic_id()

Reviewers

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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8344

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

Using diff file

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

[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 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 (34598316)
• 00: Full (5a78b445)

[vnkozlov]

Good. Need to test it.

[vnkozlov]

Testing passed.

[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:

8285394: Compiler blackholes can be eliminated due to stale ciMethod::intrinsic_id()

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

• 165f516: 8277056: Combining several C2 Print* flags asserts in xmlStream::pop_tag
• 139615b: 8285369: C2: emit reduction flag value in node and loop dumps
• 80219a4: 8284993: Replace System.exit call in swing tests with RuntimeException
• d84b9c5: 8285342: Zero build failure with clang due to values not handled in switch
• 58155a7: 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958
• f6e9ca0: 8283324: CLDRConverter run time increased by 3x
• ec4fb47: 8284189: Replace usages of 'a the' in java.desktop
• e955cac: 8285040: PPC64 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long
• 73f3e17: 8213445: jcmd VM.symboltable and VM.stringtable -verbose output contains no shared symbols or strings
• f166b5b: 8285307: remove unused os::available
• ... and 6 more: https://git.openjdk.java.net/jdk/compare/d6b5a6357710598be225e67f82e6e0c1bed2d62f...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.

[dean-long]

Wouldn't it be better to call tag_blackhole_if_possible() in Method::init_intrinsic_id()? Or is it considered an expensive operation and only the compilers (and never, say, the interpreter) would ever care about this? If it is an expensive operation, and we call it for every ciMethod, then maybe tag_blackhole_if_possible() should check for "already set" first thing.

[shipilev]

Wouldn't it be better to call tag_blackhole_if_possible() in Method::init_intrinsic_id()? Or is it considered an expensive operation and only the compilers (and never, say, the interpreter) would ever care about this?

Looking up CompilerOracle commands can be surprisingly expensive, especially with large compiler command lists. Nils optimized the lookups quite well (after all, we do a similar lookup for inline/dontinline at compilation time), but it is still not completely free. But the reason we settled to do the tagging here is that only compilers care about this intrinsic, and putting the calls to CompilerOracle in otherwise compiler-agnostic runtime code (Method) introduced the unfortunate level of cohesion.

If it is an expensive operation, and we call it for every ciMethod, then maybe tag_blackhole_if_possible() should check for "already set" first thing.

Maybe. That looks like a minor performance optimization, which I would like to keep out of this bugfixing (and what's important, more safely backportable) PR, OK? Out of curiosity, I instrumented tag_blackhole_if_possible, and on Spring startup we do about 70K lookups and "already set" filter can filter about 9K of them.

[dean-long]

OK.

[shipilev]

Thanks for reviews!

/integrate

[openjdk]

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

• 4e252cf: 8285370: Fix typo in jdk.charsets
• d8025c9: 8285362: unify os::pause platform coding
• 82f0ac0: 8284995: G1: Do not mark through Closed Archive regions during concurrent mark
• 165f516: 8277056: Combining several C2 Print* flags asserts in xmlStream::pop_tag
• 139615b: 8285369: C2: emit reduction flag value in node and loop dumps
• 80219a4: 8284993: Replace System.exit call in swing tests with RuntimeException
• d84b9c5: 8285342: Zero build failure with clang due to values not handled in switch
• 58155a7: 8285386: java/util/HashMap/WhiteBoxResizeTest.java fails in tier7 after JDK-8186958
• f6e9ca0: 8283324: CLDRConverter run time increased by 3x
• ec4fb47: 8284189: Replace usages of 'a the' in java.desktop
• ... and 9 more: https://git.openjdk.java.net/jdk/compare/d6b5a6357710598be225e67f82e6e0c1bed2d62f...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit ce8db2c.

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