On Mon, 6 May 2024 14:39:08 GMT, Claes Redestad <redes...@openjdk.org> wrote:
> This PR suggests refactoring the implementation classes of java.lang.constant
> into a new package jdk.internal.constant to enable sharing some trusted
> static factory methods with users elsewhere in java.base, such as
> java.lang.invoke and java.lang.classfile. The refactoring also adds some
> "trusted" methods for use when input is pre-validated, and makes use of them
> where applicable in the current implementation. There are more changes in the
> pipeline which will be folded into #17108 or PR'ed once that is integrated.
>
> There are two optimizations mixed up here. One in
> `MethodTypeDesc.ofDescriptor`:
>
> Name (descString)
> Cnt Base Error Test Error Unit Change
> MethodTypeDescFactories.ofDescriptor (Ljava/lang/Object;Ljava/lang/String;)I
> 6 138,371 ± 0,767 136,939 ± 1,126 ns/op 1,01x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor ()V
> 6 10,528 ± 2,495 4,110 ± 0,047 ns/op 2,56x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor ([IJLjava/lang/String;Z)Ljava/util/List;
> 6 209,390 ± 4,583 196,175 ± 3,211 ns/op 1,07x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor ()[Ljava/lang/String;
> 6 36,039 ± 8,684 20,794 ± 1,110 ns/op 1,73x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (..IIJ)V
> 6 279,139 ± 6,248 187,934 ± 0,857 ns/op 1,49x (p = 0,000*)
> MethodTypeDescFactories.ofDescriptor (.....................).
> 6 2174,387 ± 132,879 1150,652 ± 3,158 ns/op 1,89x (p = 0,000*)
> * = significant
>
>
> The other in `ClassDesc::nested`, where to get rid of a simple static method
> in `ConstantUtils` I ended up speeding up and simplifying the public factory
> method:
>
> Name Cnt Base Error Test
> Error Unit Change
> ClassDescFactories.ReferenceOnly.ofNested 6 209,853 ± 132,525 22,017 ±
> 0,573 ns/op 9,53x (p = 0,000*)
> * = significant
>
>
> The optimizations could be split out and PRd independently, but I think they
> are simple enough to include in this refactoring.
src/java.base/share/classes/java/lang/constant/ClassDesc.java line 113:
> 111: static ClassDesc ofInternalName(String name) {
> 112: validateInternalClassName(requireNonNull(name));
> 113: return ClassDesc.ofDescriptor("L" + name + ";");
We can use `ofTrusted` here and above if we validate the binary/internal names
to have no trailing or consecutive slash/dots.
src/java.base/share/classes/java/lang/constant/ClassDesc.java line 131:
> 129: */
> 130: static ClassDesc of(String packageName, String className) {
> 131: validateBinaryClassName(requireNonNull(packageName));
Suggestion:
validateBinaryClassName(packageName);
the validate call already null-checks.
src/java.base/share/classes/java/lang/constant/ClassDesc.java line 222:
> 220: }
> 221: if (desc.length() == 1 && desc.charAt(0) == 'V') {
> 222: throw new IllegalArgumentException(String.format("not a
> valid reference type descriptor: %sV", "[".repeat(rank)));
Suggestion:
throw new IllegalArgumentException(String.format("not a valid
reference type descriptor: %sV", "[".repeat(netRank)));
Or should we override this in `PrimitiveClassDescImpl`, which can bypass the
rank sum computation?
src/java.base/share/classes/jdk/internal/constant/ConstantUtils.java line 80:
> 78: char ch = name.charAt(i);
> 79: if (ch == ';' || ch == '[' || ch == '.')
> 80: throw new IllegalArgumentException("Invalid class name: "
> + name);
We can check there's no consecutive `..` `//` or trailing `.` or `/` so we can
just use the validated string to create a reference class desc.
src/java.base/share/classes/jdk/internal/constant/ReferenceClassDescImpl.java
line 68:
> 66: * @jvms 4.3.2 Field Descriptors
> 67: */
> 68: public static ReferenceClassDescImpl ofTrusted(String descriptor) {
If you named `ofTrusted` to be in parity with the MTDImpl factory, they are
different as MTD one means array is trusted; this one means descriptor is
already valid.
src/java.base/share/classes/module-info.java line 377:
> 375: exports sun.util.resources to
> 376: jdk.localedata;
> 377: exports jdk.internal.constant;
This is probably unintentional, if you do export we probably want to only
export to select modules.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591126247
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591126687
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591140187
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591133504
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591130463
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1591122920
