From 38cef2adbd956ac1e953ea7a7e7952fe093a9872 Mon Sep 17 00:00:00 2001 From: Chen Liang Date: Tue, 6 Jun 2023 15:56:36 +0000 Subject: [PATCH] 8309413: Improve the performance of MethodTypeDesc::descriptorString 8304932: MethodTypeDescImpl can be mutated by argument passed to MethodTypeDesc.of Reviewed-by: mchung --- .../java/lang/constant/MethodTypeDesc.java | 12 +- .../lang/constant/MethodTypeDescImpl.java | 71 +++++++-- .../lang/constant/MethodTypeDescTest.java | 137 ++++++++++-------- .../constant/MethodTypeDescFactories.java | 104 +++++++++++++ 4 files changed, 239 insertions(+), 85 deletions(-) create mode 100644 test/micro/org/openjdk/bench/java/lang/constant/MethodTypeDescFactories.java diff --git a/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java b/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java index 738c4d68a43..ea0c3a93764 100644 --- a/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java +++ b/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java @@ -64,7 +64,7 @@ public sealed interface MethodTypeDesc * @since 21 */ static MethodTypeDesc of(ClassDesc returnDesc) { - return new MethodTypeDescImpl(returnDesc, ConstantUtils.EMPTY_CLASSDESC); + return MethodTypeDescImpl.ofTrusted(returnDesc, ConstantUtils.EMPTY_CLASSDESC); } /** @@ -95,7 +95,7 @@ public sealed interface MethodTypeDesc * {@link ClassDesc} for {@code void} */ static MethodTypeDesc of(ClassDesc returnDesc, ClassDesc... paramDescs) { - return new MethodTypeDescImpl(returnDesc, paramDescs); + return MethodTypeDescImpl.ofTrusted(returnDesc, paramDescs.clone()); } /** @@ -195,13 +195,7 @@ public sealed interface MethodTypeDesc * @return the method type descriptor string * @jvms 4.3.3 Method Descriptors */ - default String descriptorString() { - return String.format("(%s)%s", - Stream.of(parameterArray()) - .map(ClassDesc::descriptorString) - .collect(Collectors.joining()), - returnType().descriptorString()); - } + String descriptorString(); /** * Returns a human-readable descriptor for this method type, using the diff --git a/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java b/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java index 4341c3fb56f..c50cf7c58f0 100644 --- a/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java +++ b/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -24,6 +24,8 @@ */ package java.lang.constant; +import jdk.internal.vm.annotation.Stable; + import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; import java.security.AccessController; @@ -31,6 +33,7 @@ import java.security.PrivilegedAction; import java.util.Arrays; import java.util.List; import java.util.Objects; +import java.util.StringJoiner; import static java.util.Objects.requireNonNull; @@ -41,22 +44,38 @@ import static java.util.Objects.requireNonNull; */ final class MethodTypeDescImpl implements MethodTypeDesc { private final ClassDesc returnType; - private final ClassDesc[] argTypes; + private final @Stable ClassDesc[] argTypes; + private @Stable String cachedDescriptorString; /** * Constructs a {@linkplain MethodTypeDesc} with the specified return type - * and parameter types + * and a trusted and already-validated parameter types array. * * @param returnType a {@link ClassDesc} describing the return type - * @param argTypes {@link ClassDesc}s describing the parameter types + * @param validatedArgTypes {@link ClassDesc}s describing the trusted and validated parameter types */ - MethodTypeDescImpl(ClassDesc returnType, ClassDesc[] argTypes) { + private MethodTypeDescImpl(ClassDesc returnType, ClassDesc[] validatedArgTypes) { this.returnType = requireNonNull(returnType); - this.argTypes = requireNonNull(argTypes); + this.argTypes = requireNonNull(validatedArgTypes); + } - for (ClassDesc cr : argTypes) - if (cr.isPrimitive() && cr.descriptorString().equals("V")) + /** + * Constructs a {@linkplain MethodTypeDesc} with the specified return type + * and a trusted parameter types array, which will be validated. + * + * @param returnType a {@link ClassDesc} describing the return type + * @param trustedArgTypes {@link ClassDesc}s describing the trusted parameter types + */ + static MethodTypeDescImpl ofTrusted(ClassDesc returnType, ClassDesc[] trustedArgTypes) { + Objects.requireNonNull(returnType); + if (trustedArgTypes.length == 0) // implicit null check + return new MethodTypeDescImpl(returnType, ConstantUtils.EMPTY_CLASSDESC); + + for (ClassDesc cd : trustedArgTypes) + if (cd.isPrimitive() && cd.descriptorString().charAt(0) == 'V') // implicit null check throw new IllegalArgumentException("Void parameters not permitted"); + + return new MethodTypeDescImpl(returnType, trustedArgTypes); } /** @@ -70,9 +89,18 @@ final class MethodTypeDescImpl implements MethodTypeDesc { */ static MethodTypeDescImpl ofDescriptor(String descriptor) { requireNonNull(descriptor); + List types = ConstantUtils.parseMethodDescriptor(descriptor); - ClassDesc[] paramTypes = types.stream().skip(1).map(ClassDesc::ofDescriptor).toArray(ClassDesc[]::new); - return new MethodTypeDescImpl(ClassDesc.ofDescriptor(types.get(0)), paramTypes); + + int paramCount = types.size() - 1; + var paramTypes = paramCount > 0 ? new ClassDesc[paramCount] : ConstantUtils.EMPTY_CLASSDESC; + for (int i = 0; i < paramCount; i++) { + paramTypes[i] = ClassDesc.ofDescriptor(types.get(i + 1)); + } + + MethodTypeDescImpl result = ofTrusted(ClassDesc.ofDescriptor(types.getFirst()), paramTypes); + result.cachedDescriptorString = descriptor; + return result; } @Override @@ -102,14 +130,14 @@ final class MethodTypeDescImpl implements MethodTypeDesc { @Override public MethodTypeDesc changeReturnType(ClassDesc returnType) { - return MethodTypeDesc.of(returnType, argTypes); + return new MethodTypeDescImpl(returnType, argTypes); } @Override public MethodTypeDesc changeParameterType(int index, ClassDesc paramType) { ClassDesc[] newArgs = argTypes.clone(); newArgs[index] = paramType; - return MethodTypeDesc.of(returnType, newArgs); + return ofTrusted(returnType, newArgs); } @Override @@ -120,18 +148,33 @@ final class MethodTypeDescImpl implements MethodTypeDesc { ClassDesc[] newArgs = new ClassDesc[argTypes.length - (end - start)]; System.arraycopy(argTypes, 0, newArgs, 0, start); System.arraycopy(argTypes, end, newArgs, start, argTypes.length - end); - return MethodTypeDesc.of(returnType, newArgs); + return ofTrusted(returnType, newArgs); } @Override public MethodTypeDesc insertParameterTypes(int pos, ClassDesc... paramTypes) { if (pos < 0 || pos > argTypes.length) throw new IndexOutOfBoundsException(pos); + ClassDesc[] newArgs = new ClassDesc[argTypes.length + paramTypes.length]; System.arraycopy(argTypes, 0, newArgs, 0, pos); System.arraycopy(paramTypes, 0, newArgs, pos, paramTypes.length); System.arraycopy(argTypes, pos, newArgs, pos+paramTypes.length, argTypes.length - pos); - return MethodTypeDesc.of(returnType, newArgs); + + return ofTrusted(returnType, newArgs); + } + + @Override + public String descriptorString() { + var desc = this.cachedDescriptorString; + if (desc != null) + return desc; + + var sj = new StringJoiner("", "(", ")" + returnType().descriptorString()); + for (int i = 0; i < parameterCount(); i++) { + sj.add(parameterType(i).descriptorString()); + } + return cachedDescriptorString = sj.toString(); } @Override diff --git a/test/jdk/java/lang/constant/MethodTypeDescTest.java b/test/jdk/java/lang/constant/MethodTypeDescTest.java index 482267ff33f..fd59a9240e9 100644 --- a/test/jdk/java/lang/constant/MethodTypeDescTest.java +++ b/test/jdk/java/lang/constant/MethodTypeDescTest.java @@ -24,6 +24,7 @@ import java.lang.invoke.MethodType; import java.lang.constant.ClassDesc; import java.lang.constant.MethodTypeDesc; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -32,16 +33,14 @@ import java.util.stream.Stream; import org.testng.annotations.Test; -import static java.lang.constant.ConstantDescs.CD_int; -import static java.lang.constant.ConstantDescs.CD_void; +import static java.lang.constant.ConstantDescs.*; import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; -import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertThrows; -import static org.testng.Assert.fail; +import static org.testng.Assert.*; /** * @test + * @bug 8304932 * @compile MethodTypeDescTest.java * @run testng MethodTypeDescTest * @summary unit tests for java.lang.constant.MethodTypeDesc @@ -115,12 +114,9 @@ public class MethodTypeDescTest extends SymbolicDescTest { } // try with null parameter - try { + expectThrows(NullPointerException.class, () -> { MethodTypeDesc newDesc = mtDesc.changeReturnType(null); - fail("should fail with NPE"); - } catch (NullPointerException ex) { - // good - } + }); // changeParamType for (int i=0; i(Arrays.asList(paramTypes)); + t.subList(i, j).clear(); + MethodTypeDesc multiDrop = mtDesc.dropParameterTypes(i, j); + assertEquals(multiDrop, MethodTypeDesc.of(returnType, t.toArray(ClassDesc[]::new))); + testMethodTypeDesc(multiDrop, mt.dropParameterTypes(i, j)); + } } badDropParametersTypes(CD_void, paramDescs); @@ -159,6 +164,21 @@ public class MethodTypeDescTest extends SymbolicDescTest { assertEquals(newDesc, MethodTypeDesc.of(returnType, ps)); testMethodTypeDesc(newDesc, mt.insertParameterTypes(i, p.resolveConstantDesc(LOOKUP))); } + + // add multiple params + ClassDesc[] addition = {CD_int, CD_String}; + var a = new ArrayList<>(Arrays.asList(paramTypes)); + a.addAll(i, Arrays.asList(addition)); + + MethodTypeDesc newDesc = mtDesc.insertParameterTypes(i, addition); + assertEquals(newDesc, MethodTypeDesc.of(returnType, a.toArray(ClassDesc[]::new))); + testMethodTypeDesc(newDesc, mt.insertParameterTypes(i, Arrays.stream(addition).map(d -> { + try { + return (Class) d.resolveConstantDesc(LOOKUP); + } catch (ReflectiveOperationException ex) { + throw new RuntimeException(ex); + } + }).toArray(Class[]::new))); } badInsertParametersTypes(CD_void, paramDescs); @@ -169,47 +189,32 @@ public class MethodTypeDescTest extends SymbolicDescTest { IntStream.rangeClosed(0, paramDescTypes.length - 1) .mapToObj(i -> ClassDesc.ofDescriptor(paramDescTypes[i])).toArray(ClassDesc[]::new); MethodTypeDesc mtDesc = MethodTypeDesc.of(returnType, paramTypes); - try { + expectThrows(IndexOutOfBoundsException.class, () -> { MethodTypeDesc newDesc = mtDesc.insertParameterTypes(-1, paramTypes); - fail("pos < 0 should have failed"); - } catch (IndexOutOfBoundsException ex) { - // good - } + }); - try { + expectThrows(IndexOutOfBoundsException.class, () -> { MethodTypeDesc newDesc = mtDesc.insertParameterTypes(paramTypes.length + 1, paramTypes); - fail("pos > current arguments length should have failed"); - } catch (IndexOutOfBoundsException ex) { - // good - } + }); - try { + expectThrows(IllegalArgumentException.class, () -> { ClassDesc[] newParamTypes = new ClassDesc[1]; newParamTypes[0] = CD_void; MethodTypeDesc newDesc = MethodTypeDesc.of(returnType, CD_int); newDesc = newDesc.insertParameterTypes(0, newParamTypes); - fail("shouldn't allow parameters with class descriptor CD_void"); - } catch (IllegalArgumentException ex) { - // good - } + }); - try { + expectThrows(NullPointerException.class, () -> { MethodTypeDesc newDesc = MethodTypeDesc.of(returnType, CD_int); newDesc = newDesc.insertParameterTypes(0, null); - fail("should fail with NPE"); - } catch (NullPointerException ex) { - // good - } + }); - try { + expectThrows(NullPointerException.class, () -> { ClassDesc[] newParamTypes = new ClassDesc[1]; newParamTypes[0] = null; MethodTypeDesc newDesc = MethodTypeDesc.of(returnType, CD_int); newDesc = newDesc.insertParameterTypes(0, newParamTypes); - fail("should fail with NPE"); - } catch (NullPointerException ex) { - // good - } + }); } private void badDropParametersTypes(ClassDesc returnType, String... paramDescTypes) { @@ -217,40 +222,26 @@ public class MethodTypeDescTest extends SymbolicDescTest { IntStream.rangeClosed(0, paramDescTypes.length - 1) .mapToObj(i -> ClassDesc.ofDescriptor(paramDescTypes[i])).toArray(ClassDesc[]::new); MethodTypeDesc mtDesc = MethodTypeDesc.of(returnType, paramTypes); - try { + + expectThrows(IndexOutOfBoundsException.class, () -> { MethodTypeDesc newDesc = mtDesc.dropParameterTypes(-1, 0); - fail("start index < 0 should have failed"); - } catch (IndexOutOfBoundsException ex) { - // good - } + }); - try { + expectThrows(IndexOutOfBoundsException.class, () -> { MethodTypeDesc newDesc = mtDesc.dropParameterTypes(paramTypes.length, 0); - fail("start index = arguments.length should have failed"); - } catch (IndexOutOfBoundsException ex) { - // good - } + }); - try { + expectThrows(IndexOutOfBoundsException.class, () -> { MethodTypeDesc newDesc = mtDesc.dropParameterTypes(paramTypes.length + 1, 0); - fail("start index > arguments.length should have failed"); - } catch (IndexOutOfBoundsException ex) { - // good - } + }); - try { + expectThrows(IndexOutOfBoundsException.class, () -> { MethodTypeDesc newDesc = mtDesc.dropParameterTypes(0, paramTypes.length + 1); - fail("end index > arguments.length should have failed"); - } catch (IndexOutOfBoundsException ex) { - // good - } + }); - try { + expectThrows(IndexOutOfBoundsException.class, () -> { MethodTypeDesc newDesc = mtDesc.dropParameterTypes(1, 0); - fail("start index > end index should have failed"); - } catch (IndexOutOfBoundsException ex) { - // good - } + }); } public void testMethodTypeDesc() throws ReflectiveOperationException { @@ -274,6 +265,7 @@ public class MethodTypeDescTest extends SymbolicDescTest { for (String d : badDescriptors) { assertThrows(IllegalArgumentException.class, () -> MethodTypeDesc.ofDescriptor(d)); } + assertThrows(NullPointerException.class, () -> MethodTypeDesc.ofDescriptor(null)); // of(ClassDesc) @@ -282,8 +274,6 @@ public class MethodTypeDescTest extends SymbolicDescTest { // of(ClassDesc, ClassDesc...) assertThrows(NullPointerException.class, () -> MethodTypeDesc.of(CD_int, (ClassDesc[]) null)); assertThrows(NullPointerException.class, () -> MethodTypeDesc.of(CD_int, new ClassDesc[] {null})); - // try with void arguments, this will stress another code path in particular - // ConstantMethodTypeDesc::init assertThrows(IllegalArgumentException.class, () -> MethodTypeDesc.of(CD_int, CD_void)); // of(ClassDesc, List) @@ -291,4 +281,27 @@ public class MethodTypeDescTest extends SymbolicDescTest { assertThrows(NullPointerException.class, () -> MethodTypeDesc.of(CD_int, Collections.singletonList(null))); assertThrows(IllegalArgumentException.class, () -> MethodTypeDesc.of(CD_int, List.of(CD_void))); } + + public void testOfArrayImmutability() { + ClassDesc[] args = {CD_Object, CD_int}; + var mtd = MethodTypeDesc.of(CD_void, args); + + args[1] = CD_void; + assertEquals(mtd, MethodTypeDesc.of(CD_void, CD_Object, CD_int)); + + mtd.parameterArray()[1] = CD_void; + assertEquals(mtd, MethodTypeDesc.of(CD_void, CD_Object, CD_int)); + } + + public void testOfListImmutability() { + List args = Arrays.asList(CD_Object, CD_int); + var mtd = MethodTypeDesc.of(CD_void, args); + + args.set(1, CD_void); + assertEquals(mtd, MethodTypeDesc.of(CD_void, CD_Object, CD_int)); + + assertThrows(UnsupportedOperationException.class, () -> + mtd.parameterList().set(1, CD_void)); + assertEquals(mtd, MethodTypeDesc.of(CD_void, CD_Object, CD_int)); + } } diff --git a/test/micro/org/openjdk/bench/java/lang/constant/MethodTypeDescFactories.java b/test/micro/org/openjdk/bench/java/lang/constant/MethodTypeDescFactories.java new file mode 100644 index 00000000000..27669e37e29 --- /dev/null +++ b/test/micro/org/openjdk/bench/java/lang/constant/MethodTypeDescFactories.java @@ -0,0 +1,104 @@ +/* + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ +package org.openjdk.bench.java.lang.constant; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +import java.lang.constant.ClassDesc; +import java.lang.constant.MethodTypeDesc; +import java.util.List; +import java.util.concurrent.TimeUnit; + +/** + * Performance of conversion from and to method type descriptor symbols with + * descriptor strings and class descriptor symbols + */ +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 3, time = 2) +@Measurement(iterations = 6, time = 1) +@Fork(1) +@State(Scope.Benchmark) +public class MethodTypeDescFactories { + + private static final ClassDesc DUMMY_CD = ClassDesc.of("Dummy_invalid"); + + @Param({ + "(Ljava/lang/Object;Ljava/lang/String;)I", + "()V", + "([IJLjava/lang/String;Z)Ljava/util/List;", + "()[Ljava/lang/String;" + }) + public String descString; + public MethodTypeDesc desc; + public ClassDesc ret; + public ClassDesc[] args; + public List argsList; + + @Setup + public void setup() { + desc = MethodTypeDesc.ofDescriptor(descString); + ret = desc.returnType(); + args = desc.parameterArray(); + argsList = desc.parameterList(); + } + + @Benchmark + public String descriptorString(Blackhole blackhole) { + // swaps return types with dummy classdesc; + // this shares parameter arrays and avoids revalidation + // while it drops the descriptor string cache + var mtd = desc.changeReturnType(DUMMY_CD); + blackhole.consume(mtd); + mtd = mtd.changeReturnType(desc.returnType()); + blackhole.consume(mtd); + + return mtd.descriptorString(); + } + + @Benchmark + public MethodTypeDesc ofDescriptor() { + return MethodTypeDesc.ofDescriptor(descString); + } + + @Benchmark + public MethodTypeDesc ofArray() { + return MethodTypeDesc.of(ret, args); + } + + @Benchmark + public MethodTypeDesc ofList() { + return MethodTypeDesc.of(ret, argsList); + } +}