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

8274299: Make Method/Constructor/Field accessors @Stable #5694

Closed
wants to merge 4 commits into from

Conversation

plevart
Copy link
Contributor

@plevart plevart commented Sep 25, 2021

This patch improves reflective access speed as shown by the included benchmarks:

https://jmh.morethan.io/?gists=902f4b43519c4f96c7abcd14cdc2d27d,ac490481e3001c710d75d6071c10b23a

... and is also a prerequisite to make JEP 416 (Reimplement Core Reflection with Method Handle) perform better in some circumstances.


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5694

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 25, 2021

👋 Welcome back plevart! 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 changed the title 8274299: Make Method/Constructor/Field accessors @Stable 8274299: Make Method/Constructor/Field accessors @Stable Sep 25, 2021
@openjdk openjdk bot added the rfr label Sep 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 2021

@plevart The following label will be automatically applied to this pull request:

  • core-libs

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 core-libs label Sep 25, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 25, 2021

Webrevs

cl4es
cl4es approved these changes Sep 27, 2021
Copy link
Member

@cl4es cl4es left a comment

This looks good, assuming Mandy is OK with extracting and adding to the microbenchmarks from JEP 416

A stray thought is why not most fields in Field/Method/Constructor are final, as my IDE suggests. I can't find any hotspot code that injects values to these fields. Experimentally changing most fields in Field to final seem to pass at least all the java/lang/reflect tests. Since this is a trusted package @Stable should then be pointless.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 27, 2021

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

8274299: Make Method/Constructor/Field accessors @Stable

Reviewed-by: redestad, mchung

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

  • a914ee7: 8274632: Possible pointer overflow in PretouchTask chunk claiming
  • 8f7a37c: 8274434: move os::get_default_process_handle and os::dll_lookup to os_posix for POSIX platforms
  • 3953e07: 8271459: C2: Missing NegativeArraySizeException when creating StringBuilder with negative capacity
  • 53d7e95: 8274635: Use String.equals instead of String.compareTo in jdk.accessibility
  • e43f540: 8274651: Possible race in FontDesignMetrics.KeyReference.dispose
  • 2e542e3: 8274349: ForkJoinPool.commonPool() does not work with 1 CPU
  • 7e757f6: 8274559: JFR: Typo in 'jfr help configure' text
  • 75d6688: 8274745: ProblemList TestSnippetTag.java
  • 9914e5c: 8274610: Add linux-aarch64 to bootcycle build profiles
  • 0ca094b: 8273244: Improve diagnostic output related to ErroneousTree
  • ... and 106 more: https://git.openjdk.java.net/jdk/compare/d91e227abb94953129adc297fbd456c55bb2ae10...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 Sep 27, 2021
@mlchung
Copy link
Member

@mlchung mlchung commented Sep 27, 2021

Adding to Peter's description - separating this patch from JEP 416 will give some bake time on this change of the existing core reflection implementation. When JEP 416 is integrated, it will switch the new implementation and the changes in *AccessorImpl will not be exercised. In addition, this will make the backport easier if desirable.

@mlchung
Copy link
Member

@mlchung mlchung commented Sep 27, 2021

This looks good, assuming Mandy is OK with extracting and adding to the microbenchmarks from JEP 416

I'm okay to include these microbenchmarks in this patch. I will merge the change with JEP 416.

A stray thought is why not most fields in Field/Method/Constructor are final, as my IDE suggests. I can't find any hotspot code that injects values to these fields.

See Reflection::new_method and Reflection::new_field.

Experimentally changing most fields in Field to final seem to pass at least all the java/lang/reflect tests. Since this is a trusted package @Stable should then be pointless.

This reminds to double check [1] and realized that java.lang.reflect is not in the list of trusted packages for @Stable. I'll add that in JEP 416.

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/ciField.cpp#L219

Copy link
Member

@mlchung mlchung left a comment

Looks good to me. Very nice performance improvement.

One minor comment: I think the change in UnsafeFieldAccessorImpl.java and UnsafeStaticFieldAccessorImpl.java isn't necessary since they're final fields. It can be reverted.

@mlchung
Copy link
Member

@mlchung mlchung commented Sep 27, 2021

A stray thought is why not most fields in Field/Method/Constructor are final, as my IDE suggests. I can't find any hotspot code that injects values to these fields. Experimentally changing most fields in Field to final seem to pass at least all the java/lang/reflect tests. Since this is a trusted package @Stable should then be pointless.

This is a good observation. I took another look. The only place to set those fields are in Reflection:new_field/new_method/new_constructor. Making the fields that set by the Method/Field/Constructor constructor be final is a good idea! Those final fields don't need to be @Stable.

One thing I'm puzzling is why the performance result is better but java.lang.reflect is not in the TNSFF package list.

@cl4es
Copy link
Member

@cl4es cl4es commented Sep 27, 2021

One thing I'm puzzling is why the performance result is better but java.lang.reflect is not in the TNSFF package list.

I think for deciding if a field can be treated as constant, @Stable has equal weight as a final in a TNSFF package, see https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/ciField.cpp#L287

Adding java/lang/reflect to the trusted package list seem reasonable, though.

@cl4es
Copy link
Member

@cl4es cl4es commented Sep 27, 2021

This reminds to double check [1] and realized that java.lang.reflect is not in the list of trusted packages for @Stable. I'll add that in JEP 416.

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/ciField.cpp#L219

Gotcha, I misremembered it as-if all packages under java/lang were already considered to be in that list.

@plevart
Copy link
Contributor Author

@plevart plevart commented Sep 28, 2021

Looks good to me. Very nice performance improvement.

One minor comment: I think the change in UnsafeFieldAccessorImpl.java and UnsafeStaticFieldAccessorImpl.java isn't necessary since they're final fields. It can be reverted.

There are two changes in UnsafeFieldAccessorImpl.java and UnsafeStaticFieldAccessorImpl.java. One is making fieldOffset and base (for static fields) @stable and the other is moving field one level up to FieldAccessorImpl and making it @stable. This later change is not necessary for this patch, but I noticed that you did that in JEP 416 so that field is common to all subclasses of FieldAccessorImpl including those that you are adding in JEP 416. I can undo this later change. But making fields @stable does improve performance since jdk.internal.reflect and java.lang.reflect are not final-field-trusted packages as you noted above. If JEP 416 makes them trusted, it can also remove the @stable annotations. But for backporting, they better stay there. WDYT?

@plevart
Copy link
Contributor Author

@plevart plevart commented Sep 28, 2021

A stray thought is why not most fields in Field/Method/Constructor are final, as my IDE suggests. I can't find any hotspot code that injects values to these fields. Experimentally changing most fields in Field to final seem to pass at least all the java/lang/reflect tests. Since this is a trusted package @Stable should then be pointless.

I made them final in newly added patch now. So we can either merge this or previous commit if desired.

@cl4es
Copy link
Member

@cl4es cl4es commented Sep 28, 2021

Does Reflection::new_method/... (which are natively implemented constructors) need any special treatment for them to follow the same semantics as a Java-based constructor w.r.t. final field writes? Or could they be rewritten to call the equivalent java constructor in each case?

We lack a microbenchmark for Constructor, but I suspect it might be a similar improvement to annotate the same fields in Constructor with @Stable. If not else it seems prudent to be consistent.

@plevart
Copy link
Contributor Author

@plevart plevart commented Sep 28, 2021

Does Reflection::new_method/... (which are natively implemented constructors) need any special treatment for them to follow the same semantics as a Java-based constructor w.r.t. final field writes? Or could they be rewritten to call the equivalent java constructor in each case?

I don't think they need special treatment currently. The Method/Constructor/Field instances created by native code and returned via native methods are always the root objects that are never handed to user code. They are used internally in the caches and are published to other threads via volatile field write/read (see java.lang.Class.ReflectionData). User code only sees copies of those objects which are performed using constructors.

@mlchung
Copy link
Member

@mlchung mlchung commented Sep 28, 2021

making fields @stable does improve performance since jdk.internal.reflect and java.lang.reflect are not final-field-trusted packages as you noted above. If JEP 416 makes them trusted, it can also remove the @stable annotations. But for backporting, they better stay there. WDYT?

Thanks for clarifying it. Okay to keep those changes.

I made them final in newly added patch now. So we can either merge this or previous commit if desired.

I would suggest to use the previous commit so that this patch focuses on this specific change - making the accessor @stable. We can make the fields final together with adding jdk.internal.reflect and java.lang.reflect in the trusted packages for TNSFF in JEP 416.

@cl4es
Copy link
Member

@cl4es cl4es commented Sep 28, 2021

I'm fine with going back to the previous iteration. I'd add @Stable to the same fields in Constructor, too, though.

@plevart
Copy link
Contributor Author

@plevart plevart commented Oct 2, 2021

I'm fine with going back to the previous iteration. I'd add @Stable to the same fields in Constructor, too, though.

Good catch. I'll add @stable to select Constructor fields. They are important for optimizing Const use cases. I'll also extend benchmarks to include Constructor accesses.

@plevart
Copy link
Contributor Author

@plevart plevart commented Oct 4, 2021

I'm fine with going back to the previous iteration. I'd add @Stable to the same fields in Constructor, too, though.

Good catch. I'll add @stable to select Constructor fields. They are important for optimizing Const use cases.

I checked the git history. @Stable annotation was added to Method::clazz and Method::modifiers fields as part of "JDK-8159746: (proxy) Support for default methods" to speed-up checks. But reflective access uses those two fields to perform access checks too, so it makes sense to add @stable to those fields in Field and Constructor classes too.
One thing that was missed in JDK-8159746 was the fact that Method::modifiers might have a valid zero value. In which case @stable is ineffective since JIT only optimizes the code when it encounters non-zero or non-null value. One way this could be improved is to "stamp" the modifiers field with an additional bit (say 0x8000_0000) to always hold a non-zero value. Method::getModifiers() would then clear that bit in a returned value to stay compatible. Native Method/Field/Constructor construction would have to be modified too.

@mlchung
Copy link
Member

@mlchung mlchung commented Oct 4, 2021

One way this could be improved is to "stamp" the modifiers field with an additional bit (say 0x8000_0000) to always hold a non-zero value. Method::getModifiers() would then clear that bit in a returned value to stay compatible. Native Method/Field/Constructor construction would have to be modified too.

Alternatively, it'd become a non-issue when java.lang.reflect and jdk.internal.reflect are added to the trusted final-field packages, which will be done by JEP 416.

@forax
Copy link
Member

@forax forax commented Oct 4, 2021

Can you surreptitiously add "java.lang" too (asking for a friend)

@mlchung
Copy link
Member

@mlchung mlchung commented Oct 4, 2021

java.lang is already trusted [1].

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/ciField.cpp#L219

@forax
Copy link
Member

@forax forax commented Oct 4, 2021

I miss that, that very cool.

@plevart
Copy link
Contributor Author

@plevart plevart commented Oct 4, 2021

I added Constructor benchmarks in latest commit. Here are the results:

https://jmh.morethan.io/?gists=f57a49ce833d5a53a01ce996570c11d2,de4a8a4b7ddc4bec1090d2113d9c50e6

I rest now, waiting for comments.

mlchung
mlchung approved these changes Oct 4, 2021
Copy link
Member

@mlchung mlchung left a comment

Looks good.

cl4es
cl4es approved these changes Oct 5, 2021
Copy link
Member

@cl4es cl4es left a comment

Excellent, thanks!

@plevart
Copy link
Contributor Author

@plevart plevart commented Oct 5, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 5, 2021

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

  • 1459180: 8274079: Cleanup unnecessary calls to Throwable.initCause() in java.base module
  • 8609ea5: 8273342: Null pointer dereference in classFileParser.cpp:2817
  • a5080ef: 8272564: Incorrect attribution of method invocations of Object methods on interfaces
  • a914ee7: 8274632: Possible pointer overflow in PretouchTask chunk claiming
  • 8f7a37c: 8274434: move os::get_default_process_handle and os::dll_lookup to os_posix for POSIX platforms
  • 3953e07: 8271459: C2: Missing NegativeArraySizeException when creating StringBuilder with negative capacity
  • 53d7e95: 8274635: Use String.equals instead of String.compareTo in jdk.accessibility
  • e43f540: 8274651: Possible race in FontDesignMetrics.KeyReference.dispose
  • 2e542e3: 8274349: ForkJoinPool.commonPool() does not work with 1 CPU
  • 7e757f6: 8274559: JFR: Typo in 'jfr help configure' text
  • ... and 109 more: https://git.openjdk.java.net/jdk/compare/d91e227abb94953129adc297fbd456c55bb2ae10...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Oct 5, 2021

@plevart Pushed as commit 7ad74d8.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
4 participants