8265135: Reduce work initializing VarForms #3472
Conversation
|
👋 Welcome back redestad! A progress list of the required criteria for merging this PR into |
Webrevs
|
| } catch (NoSuchMethodException | IllegalAccessException e) { | ||
| throw new UnsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
| } catch (NoSuchMethodException | IllegalAccessException e) { | |
| throw new UnsupportedOperationException(); | |
| } | |
| } catch (ReflectiveOperationException e) { | |
| throw newInternalError("Failed resolving VarHandle member name", ex); | |
| } |
There was a problem hiding this comment.
Thanks for reviewing!
Is there's a way to provoke this exception through the public API? If not then the suggested behavior change seems reasonable.
There was a problem hiding this comment.
No, since VarHandles are not publicly extensible, the exception should not occur unless something has gone very wrong (the correspondence between access mode and implementing method is broken).
There was a problem hiding this comment.
Unfortunately the change to InternalError breaks a number of tests, since the UOE does bubble up through the public API. I also found a few failing tests I had overlooked due VarHandle.isAccessModeSupported throwing rather than returning false, so I had to slightly rework the patch.
There was a problem hiding this comment.
Oops, my bad. I got confused and forgot that VH implementations can avoid implementing access mode methods that would just throw UOE. This slightly complicates lazily resolution.
We don't cache the result of a failed method resolution, which would require a non-null sentinel value, probably does not matter.
Using resolveOrNull seems a better fit rather than catching and dropping ROE, less work performed too.
|
@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: 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 51 new commits pushed to the
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 |
| MethodType type = methodType_table[value.at.ordinal()].insertParameterTypes(0, VarHandle.class); | ||
| return memberName_table[mode] | ||
| = MethodHandles.Lookup.IMPL_LOOKUP | ||
| .resolveOrFail(REF_invokeStatic, implClass, methodName, type); |
There was a problem hiding this comment.
An alternative way can be:
final MemberName getMemberName(int mode) {
MemberName mn = getMemberNameOrNull(mode);
if (mn == null) {
throw new UnsupportedOperationException();
}
}
This way resolveMemberName(int mode) can simply call IMPL_LOOKUP.resolveOrNull.
|
Thanks for reviewing, Mandy and Paul /integrate |
openjdk
bot
added
integrated
|
@cl4es Since your change was applied there have been 123 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 5303ccb. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This patch reduces work done initializing VarForms - mostly observed when loading each VarHandle implementation class.
Net effect is a reduction in bytecode executed per VH class by 50-60%.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3472/head:pull/3472$ git checkout pull/3472Update a local copy of the PR:
$ git checkout pull/3472$ git pull https://git.openjdk.java.net/jdk pull/3472/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3472View PR using the GUI difftool:
$ git pr show -t 3472Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3472.diff