On Fri, 16 Aug 2024 08:53:38 GMT, Shaojin Wen <[email protected]> wrote:
> The current implementation of ofDescriptor puts return type and parameter
> types together in an ArrayList, and then splits them into return type and
> array of parameter types. This ArrayList creation is unnecessary, considering
> most descriptors only have few parameter types.
>
> By splitting return type and parameter types separately and scanning the
> descriptor first to get the number of parameters, we can just allocate an
> exact, trusted array for the resulting MethodTypeDesc without copy.
Yes, this is a good one. I observed this oddity when I worked on #18971 but had
a long list of other improvements to work on and didn't get around to filing an
RFE.
Can you check if the pre-existing MethodTypeDescFactories microbenchmark is
sufficient to verify your optimization here?
src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 309:
> 307: private static ClassDesc resolveClassDesc(String descriptor, int
> start, int len) {
> 308: if (len == 1) {
> 309: return
> Wrapper.forPrimitiveType(descriptor.charAt(start)).basicClassDescriptor();
This was already piggy-backing on a quite fast routine. If it's a clean,
significant win then I'm fine with this (untangling `Wrapper` from this is
probably good, all things considered), but then we should also clean up
`Wrapper` to not carry some descriptors around.
src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line
109:
> 107: public static MethodTypeDescImpl ofDescriptor(String descriptor) {
> 108: // Implicit null-check of descriptor
> 109: List<ClassDesc> ptypes =
> ConstantUtils.parseMethodDescriptor(descriptor);
This might have been the only use of parseMethodDescriptor - so you can
probably remove that method, making this patch a net clean-up in terms of lines
of code.
src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line
137:
> 135: var returnType = resolveClassDesc(descriptor, rightBracket + 1,
> retTypeLength);
> 136: if (length == 3 && returnType == CD_void) {
> 137: return Constants.MTD_void;
Feels a bit like overfitting with quite limited data.
Could this use `ConstantDescs.MTD_void` instead or does that cause a bootstrap
cycle?
test/micro/org/openjdk/bench/java/lang/classfile/MethodTypeDescBench.java line
23:
> 21: * questions.
> 22: */
> 23: package org.openjdk.bench.java.lang.classfile;
Wrong package as the code being tested is in java.lang.constant. Also there is
already a related microbenchmark in
org.openjdk.bench.java.lang.constant.MethodTypeDescFactories - can you check if
that covers your needs?
-------------
PR Review: https://git.openjdk.org/jdk/pull/20611#pullrequestreview-2242492738
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1721063121
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1719707693
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1721061415
PR Review Comment: https://git.openjdk.org/jdk/pull/20611#discussion_r1719709479
