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

8271911: replay compilations of methods which use JSR292 (easy cases) #5270

Closed
wants to merge 3 commits into from

Conversation

@dean-long
Copy link
Member

@dean-long dean-long commented Aug 26, 2021

There is a subset of the general problem that we should be able to solve by looking at invokedynamic/invokehandle call sites and MethodHandle constant pool entries. If a replay references a hidden class that is discoverable in one of those locations, then we can use the location as a replacement for the transient VM name.

Examples of references to hidden class locations:

@bci compiler/ciReplay/CiReplayBase$TestMain test (I)V 1 <appendix> argL0 ;
@bci compiler/ciReplay/CiReplayBase$TestMain main ([Ljava/lang/String;)V 0 <appendix> form vmentry <vmtarget> ;
@cpi compiler/ciReplay/CiReplayBase$TestMain 56 form vmentry <vmtarget> ;

Progress

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

Issues

  • JDK-8271911: replay compilations of methods which use JSR292 (easy cases)
  • JDK-8012267: ciReplay: fails to resolve @SignaturePolymorphic methods in replay data
  • JDK-8012268: ciReplay: process_ciInstanceKlass: JVM_CONSTANT_MethodHandle not supported

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5270

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 26, 2021

👋 Welcome back dlong! 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.

@dean-long
Copy link
Member Author

@dean-long dean-long commented Aug 26, 2021

/id 8012267

@openjdk openjdk bot changed the title 8271911: replay compilations of methods which use JSR292 (easy cases) 8271911: replay compilations of methods which use JSR292 (easy cases) Aug 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 26, 2021

@dean-long Unknown command id - for a list of valid commands use /help.

@dean-long
Copy link
Member Author

@dean-long dean-long commented Aug 26, 2021

/help

@openjdk
Copy link

@openjdk openjdk bot commented Aug 26, 2021

@dean-long Available commands:

  • cc - add or remove an additional classification label
  • clean - Mark the backport pull request as a clean backport
  • contributor - adds or removes additional contributors for a PR
  • covered - used when employer has signed the OCA
  • csr - require a compatibility and specification request (CSR) for this pull request
  • help - shows this text
  • integrate - performs integration of the changes in the PR
  • issue - edit the list of issues that this PR solves
  • label - add or remove an additional classification label
  • open - Set the pull request state to "open"
  • reviewer - manage additional reviewers for a PR
  • reviewers - set the number of additional required reviewers for this PR
  • signed - used after signing the OCA
  • solves - edit the list of issues that this PR solves
  • sponsor - performs integration of a PR that is authored by a non-committer
  • summary - updates the summary in the commit message
  • test - used to run tests

@dean-long
Copy link
Member Author

@dean-long dean-long commented Aug 26, 2021

/issue 8271911, 8012267

@openjdk
Copy link

@openjdk openjdk bot commented Aug 26, 2021

@dean-long This issue is referenced in the PR title - it will now be updated.

Adding additional issue to issue list: 8012267: ciReplay: fails to resolve @SignaturePolymorphic methods in replay data.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 26, 2021

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

@dean-long
Copy link
Member Author

@dean-long dean-long commented Aug 26, 2021

/issue 8012268

@openjdk
Copy link

@openjdk openjdk bot commented Aug 26, 2021

@dean-long
Adding additional issue to issue list: 8012268: ciReplay: process_ciInstanceKlass: JVM_CONSTANT_MethodHandle not supported.

@dean-long dean-long marked this pull request as ready for review Aug 27, 2021
@openjdk openjdk bot added the rfr label Aug 27, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 27, 2021

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

In general this looks good.
My only complain is lack of comments. It difficult to understand what happens in code.

src/hotspot/share/ci/ciEnv.cpp Outdated Show resolved Hide resolved
@dean-long
Copy link
Member Author

@dean-long dean-long commented Aug 30, 2021

Thanks Vladimir for looking at this. Let me know if I missed some places that need comments.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 31, 2021

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

8271911: replay compilations of methods which use JSR292 (easy cases)
8012267: ciReplay: fails to resolve @SignaturePolymorphic methods in replay data
8012268: ciReplay: process_ciInstanceKlass: JVM_CONSTANT_MethodHandle not supported

Reviewed-by: kvn, 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 90 new commits pushed to the master branch:

  • 8c37909: 8273234: extended 'for' with expression of type tvar causes the compiler to crash
  • 28ba78e: 8244675: assert(IncrementalInline || (_late_inlines.length() == 0 && !has_mh_late_inlines()))
  • d05494f: 8266239: Some duplicated javac command-line options have repeated effect
  • 93eec9a: 8272776: NullPointerException not reported
  • 7b023a3: 8273257: jshell doesn't compile a sealed hierarchy with a sealed interface and a non-sealed leaf
  • f17ee0c: 8273263: Incorrect recovery attribution of record component type when j.l.Record is unavailable
  • fa9c865: 8273112: -Xloggc: should override -verbose:gc
  • dd87181: 8214761: Bug in parallel Kahan summation implementation
  • 7fff22a: 8272805: Avoid looking up standard charsets
  • 92b05fe: 8273251: Call check_possible_safepoint() from SafepointMechanism::process_if_requested()
  • ... and 80 more: https://git.openjdk.java.net/jdk/compare/7212561dd1ec65d7f31792959f0eaaab6229eaf4...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.

@openjdk openjdk bot added the ready label Aug 31, 2021
@dean-long
Copy link
Member Author

@dean-long dean-long commented Aug 31, 2021

Thanks Vladimir.

Copy link

@iwanowww iwanowww left a comment

Very nice work, Dean!

_dyno_locs = new (arena()) GrowableArray<const char *>(arena(), 100, 0, NULL);

// Iterate over the class hierarchy
for (ClassHierarchyIterator iter(vmClasses::Object_klass()); !iter.done(); iter.next()) {
Copy link

@iwanowww iwanowww Sep 1, 2021

Choose a reason for hiding this comment

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

Why do you iterate over the whole class hierarchy instead of inspecting only those classes which are present in CI?

Copy link
Member Author

@dean-long dean-long Sep 1, 2021

Choose a reason for hiding this comment

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

Good question. It is because of the section in ciInstanceKlass::dump_replay_data that dumps subclasses. If one of the CI classes is java.lang.Object, we can get a lot of hidden classes dumped there from startup that are unrelated to the current compile. I wanted to see how many I could find as a proof of concept / stress test. My plan is to see if we can completely do without subclass dumping there by dumping better CHA information (JDK-8261192).

Copy link

@iwanowww iwanowww Sep 2, 2021

Choose a reason for hiding this comment

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

I still miss the connection between ciInstanceKlass::dump_replay_data() and ciEnv::find_dynamic_call_sites() cases.

ciEnv::find_dynamic_call_sites() dumps all invokedynamic and invokehandle (MH.invoke*()) call sites and MethodHandle CP Constants across the class hierarchy. Any particular benefit compared to just dumping that info on per ciInstanceKlass granularity?

Copy link

@iwanowww iwanowww Sep 2, 2021

Choose a reason for hiding this comment

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

Also, one more question: why do you dump MethodHandle CP constants and invokehandle call sites? Is it to record the connection between MethodHandle instances and hidden classes behind LambdaForms they are implemented with?

Copy link
Member Author

@dean-long dean-long Sep 2, 2021

Choose a reason for hiding this comment

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

I still miss the connection between ciInstanceKlass::dump_replay_data() and ciEnv::find_dynamic_call_sites() cases.

ciEnv::find_dynamic_call_sites() dumps all invokedynamic and invokehandle (MH.invoke*()) call sites and MethodHandle CP Constants across the class hierarchy. Any particular benefit compared to just dumping that info on per ciInstanceKlass granularity?

Actually, find_dynamic_call_sites doesn't do the dumping, it just builds a map, so we only dump call sites that for hidden classes that are referenced in the replay data, preserving existing behavior. The dumping of subclasses in ciInstanceKlass::dump_replay_data confused me too, so I asked Tom Rodriguez about it. It's a substitute for CHA info in the reply file. If we load sublcasses of all the ciInstanceKlass's, then hopefully CHA queries will give the same answer at replay time. Note that not all subclasses have a ciInstanceKlass in the metadata. Subclasses of java.lang.Object include lots of hidden classes not referenced directly in the ci metadata.

Copy link
Member Author

@dean-long dean-long Sep 2, 2021

Choose a reason for hiding this comment

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

Also, one more question: why do you dump MethodHandle CP constants and invokehandle call sites? Is it to record the connection between MethodHandle instances and hidden classes behind LambdaForms they are implemented with?

Yes, if I understand your question correctly. When inlining through an invokedynamic to the target, there's often an invokehandle at the end, and the hidden class isn't always found as the adapter or appendix. There are other locations like BSM arguments, but these are loaded from the constant pool and I found that looking at MethodHandle CP constants handled that case. Note again that I don't dump all MethodHandle CP constants, just the ones needed for a hidden class referenced in the reply data.

@dean-long
Copy link
Member Author

@dean-long dean-long commented Sep 1, 2021

@iwanowww Thanks for looking at this.

Copy link

@iwanowww iwanowww left a comment

Thanks for the clarifications, Dean.

Looks good.

@dean-long
Copy link
Member Author

@dean-long dean-long commented Sep 3, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 3, 2021

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

  • d414a88: 8273240: Dynamic test ArchiveConsistency.java should use CDSArchiveUtils
  • 23fa0dc: 8272905: Consolidate discovered lists processing
  • ff4018b: 8268148: unchecked warnings handle ? and ? extends Object differently
  • 8c37909: 8273234: extended 'for' with expression of type tvar causes the compiler to crash
  • 28ba78e: 8244675: assert(IncrementalInline || (_late_inlines.length() == 0 && !has_mh_late_inlines()))
  • d05494f: 8266239: Some duplicated javac command-line options have repeated effect
  • 93eec9a: 8272776: NullPointerException not reported
  • 7b023a3: 8273257: jshell doesn't compile a sealed hierarchy with a sealed interface and a non-sealed leaf
  • f17ee0c: 8273263: Incorrect recovery attribution of record component type when j.l.Record is unavailable
  • fa9c865: 8273112: -Xloggc: should override -verbose:gc
  • ... and 83 more: https://git.openjdk.java.net/jdk/compare/7212561dd1ec65d7f31792959f0eaaab6229eaf4...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 3, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 3, 2021

@dean-long Pushed as commit 14a3ac0.

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

@dean-long dean-long deleted the 8271911 branch Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants