8266252: Streamline AbstractInterpreter::method_kind #3798

cl4es · 2021-04-29T16:04:54Z

This patch refactors AbstractInterpreter::method_kind to reduce branches on average while better compartmentalizing the exceptional cases.

Additionally Method::is_empty_method is trivial enough that making it inlineable helps reduce cost while reducing size of the libjvm.

Result is a 40% speed-up, or a reduction of .25% of instructions on Hello World that scales to larger applications.

Progress

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

Issue

JDK-8266252: Streamline AbstractInterpreter::method_kind

Reviewers

Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 4f7ea90
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 4e616b3

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3798

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

Using diff file

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

bridgekeeper · 2021-04-29T16:08:01Z

👋 Welcome back redestad! 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 · 2021-04-29T16:10:16Z

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

mlbridge · 2021-04-29T16:15:08Z

Webrevs

cl4es · 2021-04-30T22:53:18Z

/label remove hotspot
/label add hotspot-runtime

openjdk · 2021-04-30T22:53:59Z

@cl4es
The hotspot label was successfully removed.

openjdk · 2021-04-30T22:55:05Z

@cl4es
The hotspot-runtime label was successfully added.

iklam

Looks good to me. Just a minor nit about asserts.

For other reviewers, you can get better diffs by adding ?w=1 to ignore whitespace changes. I.e., https://github.com/openjdk/jdk/pull/3798/files?w=1

iklam · 2021-05-04T01:44:08Z

src/hotspot/share/interpreter/abstractInterpreter.cpp

+      case vmIntrinsics::_dabs: return java_lang_math_abs;
+      // _dsqrt will be selected for both Math::sqrt and StrictMath::sqrt, but the latter
+      // is native. Keep treating it like a native method in the interpreter
+      case vmIntrinsics::_dsqrt: return m->is_native() ? native : java_lang_math_sqrt;


Maybe we should add an assert that m must be one of the two methods mentioned by the above comment?

Ok. I'll add an assertion that allows for the StrictMath variant to be re-implemented in java

openjdk · 2021-05-04T01:46:49Z

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

8266252: Streamline AbstractInterpreter::method_kind

Reviewed-by: iklam, coleenp

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

0f925d1: 8266015: Implement AdapterHandlerLibrary lookup fast-path for common adapters
69b96f9: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux.
9b76955: 8266249: javax/swing/JPopupMenu/7156657/bug7156657.java fails on macOS
3af4efd: 8265291: Error in Javadoc for doAccessibleAction API in AccessibleJSlider class
be4f25b: 8266369: (se) Add wepoll based Selector
ff77ca8: 8266675: Optimize IntHashTable for encapsulation and ease of use
04fad70: 8266765: [BACKOUT] JDK-8255493 Support for pre-generated java.lang.invoke classes in CDS dynamic archive
0790e60: 8196743: jstatd doesn't see new Java processes inside Docker container
c6aa8f1: 8232644: bugs in serialized-form.html
b5b3119: 8266589: (fs) Improve performance of Files.copy() on macOS using copyfile(3)
... and 26 more: https://git.openjdk.java.net/jdk/compare/c665dba591ae5c15c9ca49e14d1aaa4eea38e7ae...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.

…h::sqrt

coleenp

Ok with some minor nits and questions.

coleenp · 2021-05-06T22:45:28Z

src/hotspot/share/interpreter/abstractInterpreter.cpp

-    case vmIntrinsics::_doubleToRawLongBits: return java_lang_Double_doubleToRawLongBits;
-    default:                                 break;
-  }
+    switch (m->intrinsic_id()) {


Can this just be switch (id) since you fetched m->intrinsic_id() above?

Yes, or maybe iid is a better name for that variable.

coleenp · 2021-05-06T22:51:57Z

src/hotspot/share/interpreter/abstractInterpreter.cpp

+      case vmIntrinsics::_floatToRawIntBits: return java_lang_Float_floatToRawIntBits;
+      case vmIntrinsics::_longBitsToDouble: return java_lang_Double_longBitsToDouble;
+      case vmIntrinsics::_doubleToRawLongBits: return java_lang_Double_doubleToRawLongBits;
+      case vmIntrinsics::_dsin: return java_lang_math_sin;


So ZERO doesn't have these?

Hmm, I looked at the math intrinsics, which are all unimplemented in zero. I see now that there are some intrinsics implemented in ZERO, so I'll take the conservative route and move the #ifndef to only exclude such cases as were excluded before.

coleenp · 2021-05-06T22:52:27Z

src/hotspot/share/interpreter/abstractInterpreter.cpp

+      case vmIntrinsics::_intBitsToFloat: return java_lang_Float_intBitsToFloat;
+      case vmIntrinsics::_floatToRawIntBits: return java_lang_Float_floatToRawIntBits;
+      case vmIntrinsics::_longBitsToDouble: return java_lang_Double_longBitsToDouble;
+      case vmIntrinsics::_doubleToRawLongBits: return java_lang_Double_doubleToRawLongBits;


Could you align these return statements?

Ok, I'll try tidying up - is there some rule of thumb on how to align in a switch like this where the longest case label is.. pretty long?

coleenp · 2021-05-06T22:53:32Z

src/hotspot/share/interpreter/abstractInterpreter.cpp


  // Native method?
-  // Note: This test must come _before_ the test for intrinsic
-  //       methods. See also comments below.


So apparently this is no longer true?

The one case of a native and intrinsic method is StrictMath::sqrt, which is handles specially above to retain the order of precedence.

… need to include method.inline.hpp there

cl4es · 2021-05-10T13:48:46Z

/integrate

openjdk · 2021-05-10T13:51:37Z

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

b823b3e: 8266797: Fix for 8266610 breaks the build on macos
53db2a0: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline
1603ca2: 8241248: NullPointerException in sun.security.ssl.HKDF.extract(HKDF.java:93)
0f925d1: 8266015: Implement AdapterHandlerLibrary lookup fast-path for common adapters
69b96f9: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux.
9b76955: 8266249: javax/swing/JPopupMenu/7156657/bug7156657.java fails on macOS
3af4efd: 8265291: Error in Javadoc for doAccessibleAction API in AccessibleJSlider class
be4f25b: 8266369: (se) Add wepoll based Selector
ff77ca8: 8266675: Optimize IntHashTable for encapsulation and ease of use
04fad70: 8266765: [BACKOUT] JDK-8255493 Support for pre-generated java.lang.invoke classes in CDS dynamic archive
... and 29 more: https://git.openjdk.java.net/jdk/compare/c665dba591ae5c15c9ca49e14d1aaa4eea38e7ae...master

Your commit was automatically rebased without conflicts.

Pushed as commit e41fd73.

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

cl4es added 3 commits April 28, 2021 22:39

Streamline AbstractInterpreter::method_kind

536c5cc

Select native as the kind for StrictMath::sqrt

0429ef6

Improve comment

4f7ea90

openjdk bot added the rfr Pull request is ready for review label Apr 29, 2021

openjdk bot added the hotspot hotspot-dev@openjdk.org label Apr 29, 2021

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Apr 30, 2021

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Apr 30, 2021

iklam approved these changes May 4, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 4, 2021

assert that vmIntrinsics::_dsqrt are only selected for Math/StrictMat…

4e616b3

…h::sqrt

coleenp approved these changes May 6, 2021

View reviewed changes

cl4es added 5 commits May 7, 2021 01:15

Merge branch 'master' into method_kind

0d603cb

Review comments

bb78171

Fix mismatched endifs

04af49d

assert id->iid

d06002c

8259316 added call to is_empty_method in compilerOracle.cpp so we now…

9e11dc3

… need to include method.inline.hpp there

openjdk bot closed this May 10, 2021

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 10, 2021

cl4es deleted the method_kind branch May 10, 2021 13:51

8266252: Streamline AbstractInterpreter::method_kind #3798

8266252: Streamline AbstractInterpreter::method_kind #3798

Uh oh!

Conversation

cl4es commented Apr 29, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 29, 2021

Uh oh!

openjdk bot commented Apr 29, 2021

Uh oh!

mlbridge bot commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

cl4es commented Apr 30, 2021

Uh oh!

openjdk bot commented Apr 30, 2021

Uh oh!

openjdk bot commented Apr 30, 2021

Uh oh!

iklam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cl4es commented May 10, 2021

Uh oh!

openjdk bot commented May 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

cl4es commented Apr 29, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 29, 2021 •

edited

Loading

openjdk bot commented May 4, 2021 •

edited

Loading