RFR: 8331724: Refactor j.l.constant implementation to internal package

[Chen Liang]

On Mon, 6 May 2024 14:39:08 GMT, Claes Redestad <redestad at 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