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

8318609: Upcall stubs should be smaller #16290

Closed

Conversation

TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Oct 20, 2023

We can use resolve_global_jobject (introduced by JDK-8299089) because we know that the handle is created by JNIHandles::make_global in UL_MakeUpcallStub. That creates much shorter code.

Size reduced from 1360 bytes to 648 bytes on x64 for a trivial upcall using G1.
1584 bytes to 936 bytes using ShenandoahGC.
1064 bytes to 848 bytes using Generational ZGC.

We can get even smaller size by moving JNIHandles::resolve(receiver) into UpcallLinker::on_entry (2nd commit):
616 bytes regardless of GC.
This was originally proposed here: #12708 (comment)

Moving exception handling into a shared stub saves a bit more. Got 576 bytes with 3rd commit.


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-8318609: Upcall stubs should be smaller (Enhancement - P4)

Reviewers

Contributors

  • Jorn Vernee <jvernee@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16290

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 20, 2023

👋 Welcome back mdoerr! 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 Oct 20, 2023
@openjdk
Copy link

openjdk bot commented Oct 20, 2023

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

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 20, 2023
@mlbridge
Copy link

mlbridge bot commented Oct 20, 2023

Webrevs

@TheRealMDoerr
Copy link
Contributor Author

@sid8606: resolve_global_jobject from JDK-8299089 is not yet available on s390. That's why I omitted s390.

@TheRealMDoerr
Copy link
Contributor Author

@feilongjiang: I didn't test it on riscv.

@feilongjiang
Copy link
Member

feilongjiang commented Oct 20, 2023

As I remember it was measured before: #12908 (comment)

I would take another look.

@TheRealMDoerr
Copy link
Contributor Author

As I remember it was measured before: #12908 (comment)

I would take another look.

You have convinced me that resolve_global_jobject is still too long for some GCs. I've moved it into the C code.

@TheRealMDoerr
Copy link
Contributor Author

@sid8606, @feilongjiang: Latest version is implemented for all platforms, but I have only tested on x64, ppc64 and aarch64.

Copy link
Member

@reinrich reinrich left a comment

Choose a reason for hiding this comment

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

Hi Martin,
changes look good to me.
Thanks, Richard.

@openjdk
Copy link

openjdk bot commented Oct 23, 2023

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

8318609: Upcall stubs should be smaller

Co-authored-by: Jorn Vernee <jvernee@openjdk.org>
Reviewed-by: rrich, jvernee

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

  • e6f23a9: 8315024: Vector API FP reduction tests should not test for exact equality
  • fd332da: 8317289: javadoc fails with -sourcepath if module-info.java contains import statements
  • 6d3cb45: 8318591: avoid leaks in loadlib_aix.cpp reload_table()
  • cb383c0: 8318587: refresh libraries cache on AIX in print_vm_info
  • 4bfe226: 8310031: Parallel: Implement better work distribution for large object arrays in old gen
  • 08f7914: 8305753: Allow JIT compilation for -Xshare:dump
  • 728b858: 8318130: SocksSocketImpl needlessly encodes hostname for IPv6 addresses
  • eb59167: 8318691: runtime/CompressedOops/CompressedClassPointersEncodingScheme.java fails with release VMs
  • 1b15011: 8318476: Add resource consumption note to BigInteger and BigDecimal
  • 5ba9705: 8318485: Narrow klass shift should be zero if encoding range extends to 0x1_0000_0000
  • ... and 34 more: https://git.openjdk.org/jdk/compare/40106422bd2ae3da98d028bdbab2c240a71081e3...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 23, 2023
@TheRealMDoerr
Copy link
Contributor Author

Thanks for the review!
I have found an s390 machine for testing in the meantime. jdk/java/foreign have passed.
Only riscv testing is missing, now.

Copy link
Member

@JornVernee JornVernee left a comment

Choose a reason for hiding this comment

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

Hey Martin, the changes look good.

Another thing we could do to reduce the size of the upcall stubs is move the exception handler to stub routines. The exception handler is the same for every upcall stub, but we generate one for each.

I have a POC for x64/aarch64 that I've rebased that moves the exception handler out of line: master...JornVernee:jdk:Shared_E_Handler feel free to use that if you want. Or, I can address that separately.

@feilongjiang
Copy link
Member

Sorry for the late reply, all tests under jdk_foreign passed with linux-riscv fastdebug build (w/and w/o UseRVC).

@TheRealMDoerr
Copy link
Contributor Author

@JornVernee: Thanks for the feedback! I've added your code and implemented it for all platforms.
@feilongjiang: Thanks for testing! Would you mind retesting the latest version?

@TheRealMDoerr
Copy link
Contributor Author

/contributor add JornVernee

@openjdk
Copy link

openjdk bot commented Oct 23, 2023

@TheRealMDoerr JornVernee was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

@TheRealMDoerr
Copy link
Contributor Author

/contributor add jvernee

@openjdk
Copy link

openjdk bot commented Oct 23, 2023

@TheRealMDoerr
Contributor Jorn Vernee <jvernee@openjdk.org> successfully added.

@feilongjiang
Copy link
Member

@feilongjiang: Thanks for testing! Would you mind retesting the latest version?

jdk_foreign tests are still good with new changes.

Copy link
Member

@reinrich reinrich left a comment

Choose a reason for hiding this comment

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

Looks still good to me.
Cheers, Richard.

@TheRealMDoerr
Copy link
Contributor Author

Thanks for the reviews, contribution and testing!
/integrate

@openjdk
Copy link

openjdk bot commented Oct 24, 2023

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

  • e6f23a9: 8315024: Vector API FP reduction tests should not test for exact equality
  • fd332da: 8317289: javadoc fails with -sourcepath if module-info.java contains import statements
  • 6d3cb45: 8318591: avoid leaks in loadlib_aix.cpp reload_table()
  • cb383c0: 8318587: refresh libraries cache on AIX in print_vm_info
  • 4bfe226: 8310031: Parallel: Implement better work distribution for large object arrays in old gen
  • 08f7914: 8305753: Allow JIT compilation for -Xshare:dump
  • 728b858: 8318130: SocksSocketImpl needlessly encodes hostname for IPv6 addresses
  • eb59167: 8318691: runtime/CompressedOops/CompressedClassPointersEncodingScheme.java fails with release VMs
  • 1b15011: 8318476: Add resource consumption note to BigInteger and BigDecimal
  • 5ba9705: 8318485: Narrow klass shift should be zero if encoding range extends to 0x1_0000_0000
  • ... and 34 more: https://git.openjdk.org/jdk/compare/40106422bd2ae3da98d028bdbab2c240a71081e3...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Oct 24, 2023

@TheRealMDoerr Pushed as commit a644670.

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

@TheRealMDoerr TheRealMDoerr deleted the 8318609_Upcall_stubs branch October 24, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
4 participants