8309413: Improve the performance of MethodTypeDesc::descriptorString #13186
Conversation
|
👋 Welcome back liach! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
Additional comments:
static MethodTypeDesc of(ClassDesc returnType, List<ClassDesc> parameterTypes) {
return new MethodTypeDescImpl(returnType, List.copyOf(parameterTypes));
}which would avoid another copy if
|
Yes, any improvements avoiding unnecessary arrays copying are welcome. |
|
Have you considered that the caller of instantiating That way will avoid using |
My plan was that static MethodTypeDesc of(ClassDesc returnType) {
return of(returnType, List.of());
}
static MethodTypeDesc of(ClassDesc returnType, Collection<ClassDesc> paramTypes) {
return of(returnType, List.copyOf(paramTypes));
}The For the backing structure for parameters, I still believe an immutable list is better than an array; unlike MethodType that needs to interact with the VM which loves arrays better, MTD is mainly user-oriented and |
liach
changed the title
|
/issue JDK-8304932 Moved this patch to another issue that focus on converting the storage from array to immutable list. The fix for mutation is now a side effect of this patch. |
|
@liach |
|
|
|
Now updated to the latest MethodTypeDesc.of factories, this patch is ready for review and integration. Below is a few performance information about this patch. This patch: master: The Classfile API code generation and descriptor string computation speed up with this patch. Additional benchmarks for this patch: Shows that immutable parameter lists from users can be reused, and that using object representation to construct method type descs is always preferable to parsing descriptors; this is the same conclusion from #13671, that avoiding repeated parsing and reusing tokenization from classfile builder site actually improves performance. Descriptor parsing is somewhat slower than in the initial benchmark, though the recent updates did not touch code in that area. |
|
The microbenchmark shows the performance of using the JDK-8304932 is a bug that can simply be fixed by I won't object to keep So I think we should only fix JDK-8304932. |
|
The benefits of a list implementation over an array implementation are:
The parsing is no more, but In fact, I don't think argType array sharing goes far enough: in these modification methods and ofDescriptor, we can skip the array/list iteration validation if we can ensure the change parts alone have not produced an invalid method type (insertion of void, slot changes). I would postpone such in-depth sharing into a later issue, where we track the slot count information as well (to preemptively reject invalid resolveConstantDesc calls)
Unfortunately, this is currently the only way general users have access to frozen arrays, where index-based access may be constant-folded, as |
liach
changed the title
|
@liach Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
@mlchung I have updated this patch to no longer change the array to a list. Could you take a look? I wish this to be integrated into JDK 21 as it does bring a lot of performance benefits. |
There was a problem hiding this comment.
Some comments in MethodTypeDesc and its implementation. Will take a look at the test.
I'd like to see the ClassFile implementation changes in a separate PR that needs @asotona to give input and review. The proposed change in the ClassFile implementation is trivial but I can't really tell what the performance issue is (of course, I know it avoids the array allocation).
For ClassFile performance issues, it would be useful to have the understanding of the major issues that contribute to the performance overhead. That would help consider the fixes and alternatives.
| @@ -95,7 +96,12 @@ static MethodTypeDesc of(ClassDesc returnDesc, List<ClassDesc> paramDescs) { | |||
| * {@link ClassDesc} for {@code void} | |||
| */ | |||
| static MethodTypeDesc of(ClassDesc returnDesc, ClassDesc... paramDescs) { | |||
| return new MethodTypeDescImpl(returnDesc, paramDescs); | |||
| if (paramDescs.length == 0) | |||
| .map(ClassDesc::descriptorString) | ||
| .collect(Collectors.joining()), | ||
| returnType().descriptorString()); | ||
| var sj = new StringJoiner("", "(", ")" + returnType().descriptorString()); |
There was a problem hiding this comment.
MethodTypeDescImpl is the only class implementing MethodTypeDesc and implements descriptorString(). This default implementation is not needed. Should the implementation be moved to MethodTypeDescImpl::descriptorString?
| if (paramCount > 0) { | ||
| paramTypes = new ClassDesc[paramCount]; | ||
| for (int i = 0; i < paramCount; i++) { | ||
| paramTypes[i] = validateParameter(ClassDesc.ofDescriptor(types.get(i + 1))); |
There was a problem hiding this comment.
It seems useful to have a static factory method to take a trusted copy of parameter types and it will validate the parameters before constructing MethodTypeDescImpl instance. Parameter validation in one single place. Several methods doing the validation can simply call this factory method and the code would be cleaner.
insertParameterTypes can call it as well and overhead of re-validating existing parameter types isn't a big issue.
There was a problem hiding this comment.
Should I just keep track of the slot count of MethodTypeDesc instead? Having a slot count implicitly requires validating the parameters, and the slot count can be used to eagerly reject resolveConstantDesc on MTD with over 255 slots.
There was a problem hiding this comment.
I don't think resolveConstantDesc is performance-sensitive that needs the eager checking of the number of parameter slots. Also the common case is to resolve a good MethodType with <= 255 parameter slots.
| @@ -62,14 +61,14 @@ private void testMethodTypeDesc(MethodTypeDesc r) throws ReflectiveOperationExce | |||
| assertEquals(r, MethodTypeDesc.of(r.returnType(), r.parameterList().toArray(new ClassDesc[0]))); | |||
| assertEquals(r, MethodTypeDesc.of(r.returnType(), r.parameterList().stream().toArray(ClassDesc[]::new))); | |||
| assertEquals(r, MethodTypeDesc.of(r.returnType(), IntStream.range(0, r.parameterCount()) | |||
| .mapToObj(r::parameterType) | |||
| .toArray(ClassDesc[]::new))); | |||
| .mapToObj(r::parameterType) | |||
There was a problem hiding this comment.
this reformatting may be accidental. Please keep the original format. Same applies to other reformatted lines in this file.
|
I added an |
| */ | ||
| MethodTypeDescImpl(ClassDesc returnType, ClassDesc[] argTypes) { | ||
| MethodTypeDescImpl(ClassDesc returnType, ClassDesc[] validatedArgTypes) { |
There was a problem hiding this comment.
Can make this constructor private to prevent accidental use with invalidated argTypes. MethodTypeDesc.of(ClassDesc returnDesc) can call ofTrusted factory method.
src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Mandy Chung <mandy.chung@oracle.com>
liach
changed the title
|
@liach 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 22 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mlchung) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
/sponsor |
|
Going to push as commit 38cef2a.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
This patch aims to improve the performance of MethodTypeDesc in general, as it is currently a performance bottleneck in the Classfile API. A previous revision changed the parameter storage from an array to a list; this is dropped now. Sorry for the force push.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13186/head:pull/13186$ git checkout pull/13186Update a local copy of the PR:
$ git checkout pull/13186$ git pull https://git.openjdk.org/jdk.git pull/13186/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13186View PR using the GUI difftool:
$ git pr show -t 13186Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13186.diff
Webrev
Link to Webrev Comment