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

8218885: Restore pop_frame and force_early_return functionality for Graal #5625

Closed
wants to merge 1 commit into from

Conversation

tkrodriguez
Copy link
Contributor

@tkrodriguez tkrodriguez commented Sep 22, 2021

This logic no longer seems to be necessary since the adjustCompilationLevel callback has been removed.


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-8218885: Restore pop_frame and force_early_return functionality for Graal

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5625

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 22, 2021

👋 Welcome back never! 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 openjdk bot added the rfr Pull request is ready for review label Sep 22, 2021
@openjdk
Copy link

openjdk bot commented Sep 22, 2021

@tkrodriguez The following labels will be automatically applied to this pull request:

  • hotspot
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Sep 22, 2021
@mlbridge
Copy link

mlbridge bot commented Sep 22, 2021

Webrevs

@sspitsyn
Copy link
Contributor

Hi Tom,
The fix looks good in general.
We disabled can_pop_frame and can_pop_early_return capabilities with Graal because some jvmti jck tests failed intermittently. So, I'll submit jvmti jck tests on mach5 to make sure they do not fail anymore.

@tkrodriguez
Copy link
Contributor Author

Since Graal is gone I don't think it's possible to run those tests any more. It might be possible to run those tests on some source base after https://bugs.openjdk.java.net/browse/JDK-8219403 when adjustCompilationLevel was removed.

@sspitsyn
Copy link
Contributor

Yes, I got errors since Graal is gone:
JVMCI compiler 'graal' specified by jvmci.Compiler not found

If you have a repository with Graal can you run this mach5 command? :

mach5 remote-build-and-test --email tom.rodriguez@oracle.com \
  --id-tag jck-jvmti-graal-Xcomp --comment jvmti-graal-Xcomp --test "jck:vm/jvmti" \
  -a "-Xcomp -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -XX:+TieredCompilation -XX:+UseJVMCICompiler -Djvmci.Compiler=graal" \
  -b linux-x64-debug,windows-x64-debug,macosx-x64-debug,linux-x64,windows-x64,macosx-x64

Otherwise, I'll try to extract a jdk 13 version with the adjustCompilationLevel removed.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

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

@tkrodriguez Did you test this changes with GraalVM?
Would be nice to run it with command line which Serguei pointed.
We will be fine if it passed with changes.

@tkrodriguez
Copy link
Contributor Author

I guess I'm not clear how I'm supposed to use that command line to test GraalVM. There's no JDK repository that contains both these JVMTI changes and Graal. Are these tests that can be run in 11? The JDK 11 repository doesn't have this change and also has adjustCompilationLevel removed so if those tests passed there it would be evidence that these changes can be removed. cc @dougxc can you know how to test a JDK17 GraalVM using that command line?

@dougxc
Copy link
Member

dougxc commented Sep 30, 2021

I sent instructions via email since it requires using internal resources.

@tkrodriguez
Copy link
Contributor Author

I don't see any easy way to run the suggested command at the moment through mach5. We've already pushed this change into our JDK17 repos and once we have a snapshot GraalVM based on that I could do it, but it will probably take at least 2-3 weeks for all those bits to be promoted.
Why are we running the jck jvmti tests on this since they weren't failing originally? The problem test list from the commit http://hg.openjdk.java.net/jdk/jdk12/rev/f5fd8eefae0f is all nsk and com.sun.jdi tests. I ran those locally with jtreg using a GraalVM built from a patched JDK and they all passed. Is that sufficient?

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

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

Yes, running locally with GraalVM is fine.

@openjdk
Copy link

openjdk bot commented Oct 1, 2021

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

8218885: Restore pop_frame and force_early_return functionality for Graal

Reviewed-by: kvn, dlong, sspitsyn, amenkov

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

  • dd9aa72: 8296083: javax/swing/JTree/6263446/bug6263446.java fails intermittently on a VM
  • cc44419: 8295407: C2 crash: Error: ShouldNotReachHere() in multiple vector tests with -XX:-MonomorphicArrayCheck -XX:-UncommonNullCast
  • e2269fd: 8296968: Update langtools tests to use @enablePreview
  • 2159170: 8296453: Simplify resource_area uses in ClassPathDirEntry::open_stream
  • 95c390e: 8296956: [JVMCI] HotSpotResolvedJavaFieldImpl.getIndex returns wrong value
  • 68d3ed5: 8296442: EncryptedPrivateKeyInfo can be created with an uninitialized AlgorithmParameters
  • 37848a9: 8296967: [JVMCI] rationalize relationship between getCodeSize and getCode in ResolvedJavaMethod
  • b3ef337: 8296960: [JVMCI] list HotSpotConstantPool.loadReferencedType to ConstantPool
  • f0474b8: 8283238: make/scripts/compare.sh should show the diff when classlist does not match
  • 04a4d34: 8297006: JFR: AbstractEventStream should not hold thread instance
  • ... and 10 more: https://git.openjdk.org/jdk/compare/8b1ff9e37efc42aeb05170463ec330c221ce1e4c...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 Pull request is ready to be integrated label Oct 1, 2021
@dean-long
Copy link
Member

Make sure to test with -Xcomp.

@sspitsyn
Copy link
Contributor

sspitsyn commented Oct 1, 2021

Why are we running the jck jvmti tests on this since they weren't failing originally? The problem test list from the commit http://hg.openjdk.java.net/jdk/jdk12/rev/f5fd8eefae0f is all nsk and com.sun.jdi tests.

The reason to disable PopFrame and ForceEarlyReturn with Graal were failing jck tests. They are not JTreg tests, handled differently and have no ProblemList.
I'd suggest to integrate your fix now and deal with this potential problem when Graal is back.

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 30, 2021

@tkrodriguez 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!

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 27, 2021

@tkrodriguez This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Nov 27, 2021
@tkrodriguez
Copy link
Contributor Author

/open

@openjdk openjdk bot reopened this Nov 16, 2022
@openjdk
Copy link

openjdk bot commented Nov 16, 2022

@tkrodriguez This pull request is now open

@tkrodriguez
Copy link
Contributor Author

mach5 runs with the requested options on a GraalVM have passed so I'm going to merge this now. Sound good?

@vnkozlov
Copy link
Contributor

mach5 runs with the requested options on a GraalVM have passed so I'm going to merge this now. Sound good?

Yes

@sspitsyn
Copy link
Contributor

Yes.

@tkrodriguez
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Nov 17, 2022

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

  • dd9aa72: 8296083: javax/swing/JTree/6263446/bug6263446.java fails intermittently on a VM
  • cc44419: 8295407: C2 crash: Error: ShouldNotReachHere() in multiple vector tests with -XX:-MonomorphicArrayCheck -XX:-UncommonNullCast
  • e2269fd: 8296968: Update langtools tests to use @enablePreview
  • 2159170: 8296453: Simplify resource_area uses in ClassPathDirEntry::open_stream
  • 95c390e: 8296956: [JVMCI] HotSpotResolvedJavaFieldImpl.getIndex returns wrong value
  • 68d3ed5: 8296442: EncryptedPrivateKeyInfo can be created with an uninitialized AlgorithmParameters
  • 37848a9: 8296967: [JVMCI] rationalize relationship between getCodeSize and getCode in ResolvedJavaMethod
  • b3ef337: 8296960: [JVMCI] list HotSpotConstantPool.loadReferencedType to ConstantPool
  • f0474b8: 8283238: make/scripts/compare.sh should show the diff when classlist does not match
  • 04a4d34: 8297006: JFR: AbstractEventStream should not hold thread instance
  • ... and 10 more: https://git.openjdk.org/jdk/compare/8b1ff9e37efc42aeb05170463ec330c221ce1e4c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 17, 2022
@openjdk openjdk bot closed this Nov 17, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 17, 2022
@openjdk
Copy link

openjdk bot commented Nov 17, 2022

@tkrodriguez Pushed as commit d61720a.

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

@openjdk-notifier
Copy link

@tkrodriguez Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

@tkrodriguez tkrodriguez deleted the tkr-pop-frame branch November 17, 2022 20:52
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 serviceability serviceability-dev@openjdk.org
6 participants