Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8259316: [REDO] C1/C2 compiler support for blackholes #2024

Closed
wants to merge 27 commits into from

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Jan 11, 2021

This reworks the compiler support for blackholes. The key difference against the last version (#1203) is that blackholes are only acceptable as empty static methods, which both simplifies the implementation and eliminates a few compatibility questions.

JMH uses the Blackhole::consume methods to avoid dead-code elimination of the code that produces benchmark values. It now relies on producing opaque side-effects and breaking inlining. While it was proved useful for many years, it unfortunately comes with several major drawbacks.

Instead of introducing public APIs or special-casing JMH methods in JVM, we can hook a new command to compiler control, and let JMH sign up its Blackhole methods for it with -XX:CompileCommand=blackhole,org.openjdk.jmh.infra.Blackhole::consume. See CSR and related discussion for alternatives and future plans.

C1 code is platform-independent, and it handles blackhole via the intrinsics paths, lowering it to nothing.

C2 makes the Blackhole the subclass of MemBar, and use the same Matcher path as Op_MemCPUOrder: it does not match to anything, but it survives until matching, and keeps arguments alive. Additionally, C1 and C2 hooks are now using the synthetic _blackhole intrinsic, similarly to existing _compiledLambdaForm. It avoids introducing new nodes in C1. It also seems to require the least fiddling with C2 internals.


Progress

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

Issue

  • JDK-8259316: [REDO] C1/C2 compiler support for blackholes

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2024

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 11, 2021

👋 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
Copy link

openjdk bot commented Jan 11, 2021

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

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Jan 11, 2021
@shipilev shipilev force-pushed the JDK-8259316-blackholes-redo branch from b23b5d1 to bcd90c4 Compare Jan 14, 2021
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 8, 2021

@shipilev This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@shipilev
Copy link
Member Author

shipilev commented Feb 8, 2021

Not yet, bot.

src/hotspot/share/ci/ciMethod.cpp Outdated Show resolved Hide resolved
@shipilev shipilev marked this pull request as ready for review Mar 31, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 31, 2021
@mlbridge
Copy link

mlbridge bot commented Mar 31, 2021

Webrevs

@shipilev
Copy link
Member Author

shipilev commented Apr 21, 2021

CSR is approved, I would appreciate some code reviews.

@dougxc
Copy link
Member

dougxc commented Apr 21, 2021

JVMCI changes look good.

Copy link

@iwanowww iwanowww left a comment

Still looks good.

@openjdk
Copy link

openjdk bot commented Apr 21, 2021

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

8259316: [REDO] C1/C2 compiler support for blackholes

Reviewed-by: vlivanov, kvn, iignatyev

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

  • 02cdd6e: 8266419: [aix] in mmap mode, os::attempt_reserve_memory_at() fails to handle wrong mapping address
  • 250b45a: 8266222: [aix] In mmap-mode, partial releases with os::release_memory may trash internal bookkeeping

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 openjdk bot added the ready Pull request is ready to be integrated label Apr 21, 2021
Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good. Please, update copyright year in files you touched.

@iignatev
Copy link
Member

iignatev commented Apr 21, 2021

@shipilev , as your tests ignore external vm flags you should add @requires vm.flagless so we won't waste time on running them with different GC, JITs, etc.

to restore the balance in test descriptions, you might want to delete @build compiler.blackhole.BlackholeTarget as there is a statically detectable dependency b/w compiler.blackhole.*Test and BlackholeTarget hence jtreg/javac will build it while building compiler.blackhole.*Test

Thanks,
-- Igor

@shipilev
Copy link
Member Author

shipilev commented Apr 22, 2021

Good. Please, update copyright year in files you touched.

Thanks, only a few files needed updates to 2021.

@shipilev
Copy link
Member Author

shipilev commented Apr 22, 2021

@shipilev , as your tests ignore external vm flags you should add @requires vm.flagless so we won't waste time on running them with different GC, JITs, etc.

Right. Added!

to restore the balance in test descriptions, you might want to delete @build compiler.blackhole.BlackholeTarget as there is a statically detectable dependency b/w compiler.blackhole.*Test and BlackholeTarget hence jtreg/javac will build it while building compiler.blackhole.*Test

I don't think that's true... When I remove @build from those tests, they fail with missing BlackholeTarget.

@iignatev
Copy link
Member

iignatev commented Apr 22, 2021

to restore the balance in test descriptions, you might want to delete @build compiler.blackhole.BlackholeTarget as there is a statically detectable dependency b/w compiler.blackhole.*Test and BlackholeTarget hence jtreg/javac will build it while building compiler.blackhole.*Test

I don't think that's true... When I remove @build from those tests, they fail with missing BlackholeTarget.

right, that's b/c you need to add / into the list of libraries:

- * @library /test/lib
- * @build compiler.blackhole.BlackholeTarget
+ * @library /test/lib /

@shipilev
Copy link
Member Author

shipilev commented Apr 22, 2021

- * @library /test/lib
- * @build compiler.blackhole.BlackholeTarget
+ * @library /test/lib /

I see! Did so in the new commit.

@shipilev
Copy link
Member Author

shipilev commented May 5, 2021

/integrate

@openjdk openjdk bot closed this May 5, 2021
@openjdk 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 May 5, 2021
@openjdk
Copy link

openjdk bot commented May 5, 2021

@shipilev Since your change was applied there have been 9 commits pushed to the master branch:

  • f07bb2f: 8250766: javadoc adds redundant spaces when @see program element is wrapped
  • 61bb6ec: 8266453: Shenandoah: Disable write protections before patching nmethod in nmethod_barrier on MacOSX/AArch64
  • a05e8e2: 8266497: Remove unnecessary EMCP liveness indication
  • 6ba911d: 8266498: Make debug ps() call print_stack
  • ef0f693: 8266499: Delete dead code in aarch64.ad
  • 71d0858: 8266522: Shenandoah: Shenandoah LRB calls wrong runtime barrier on aarch64
  • 0ac84d1: 8266506: [aix] Treat mapping attempt too close to BRK as a mapping error
  • 02cdd6e: 8266419: [aix] in mmap mode, os::attempt_reserve_memory_at() fails to handle wrong mapping address
  • 250b45a: 8266222: [aix] In mmap-mode, partial releases with os::release_memory may trash internal bookkeeping

Your commit was automatically rebased without conflicts.

Pushed as commit 6018336.

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

@tkrodriguez
Copy link
Contributor

tkrodriguez commented May 5, 2021

Due to my misunderstanding the github review process I had a pending comment here which was never posted. In this code, 6018336#diff-fa2433a762244542fec57f9d58dd3092bae74f354acf0ef33603a5f8306fd7daR995, the call to CompilerOracle::tag_blackhole_if_possible should be outside of the if. As written, methods won't be properly tagged for libgraal, only when used with the pure Java graal.

@shipilev
Copy link
Member Author

shipilev commented May 5, 2021

the call to CompilerOracle::tag_blackhole_if_possible should be outside of the if.

Oh. See the follow-up in #3887.

@shipilev shipilev deleted the JDK-8259316-blackholes-redo branch May 11, 2021
@@ -154,6 +154,8 @@ ciMethod::ciMethod(const methodHandle& h_m, ciInstanceKlass* holder) :
ciReplay::initialize(this);
}
#endif

CompilerOracle::tag_blackhole_if_possible(h_m);
Copy link

@danilaml danilaml Mar 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for why not updating _intrinsic_id = h_m->intrinsic_id(); with the potentially updated id after calling this is fine?
Wouldn't it still contain the old id set here (at least for the first instance): https://github.com/openjdk/jdk/pull/2024/files#diff-c08baec11bce860df907b1edd944889e3778519fdd7c7a5b9d00e10eb267667fR84 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
7 participants