Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v5]

2024-05-08 Thread Claes Redestad 
> 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,68420,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  CntBase ErrorTest   
> 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.

Claes Redestad has updated the pull request incrementally with one additional 
commit since the last revision:

  Refactor to further avoid re-validating arguments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19105/files
  - new: https://git.openjdk.org/jdk/pull/19105/files/543d0709..eb23cf51

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=03-04

  Stats: 33 lines in 2 files changed: 21 ins; 0 del; 12 mod
  Patch: https://git.openjdk.org/jdk/pull/19105.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19105/head:pull/19105

PR: https://git.openjdk.org/jdk/pull/19105

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v5]

2024-05-08 Thread ExE Boss 
On Wed, 8 May 2024 10:42:27 GMT, Claes Redestad  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,68420,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  CntBase ErrorTest   
>> 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.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refactor to further avoid re-validating arguments

src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line 
179:

> 177: 
> 178: for (ClassDesc param : paramTypes)
> 179: validateArgument(param);

This check should be performed as part of copying `paramTypes` to `newArgs` to 
avoid TOC/TOU issues, e.g.:

@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];
if (pos > 0) {
System.arraycopy(argTypes, 0, newArgs, 0, pos);
}
for (int i = 0; i < paramTypes.length; i++) {
newArgs[pos + i] = validateArgument(paramTypes[i]);
}
if (pos < argTypes.length) {
System.arraycopy(argTypes, pos, newArgs, pos + 
paramTypes.length, argTypes.length - pos);
}
return ofValidated(returnType, newArgs);
}


See also:
https://github.com/openjdk/jdk/blob/230fac80f25e9608006c8928a8a7708bf13a818c/src/java.base/share/classes/java/util/ImmutableCollections.java#L186-L195

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1594510390

Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v5]

2024-05-08 Thread Claes Redestad 
On Wed, 8 May 2024 19:06:35 GMT, ExE Boss  wrote:

>> Claes Redestad has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Refactor to further avoid re-validating arguments
>
> src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java 
> line 179:
> 
>> 177: 
>> 178: for (ClassDesc param : paramTypes)
>> 179: validateArgument(param);
> 
> This check should be performed as part of copying `paramTypes` to `newArgs` 
> to avoid TOC/TOU issues, e.g.:
> 
> @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];
>   if (pos > 0) {
>   System.arraycopy(argTypes, 0, newArgs, 0, pos);
>   }
>   for (int i = 0; i < paramTypes.length; i++) {
>   newArgs[pos + i] = validateArgument(paramTypes[i]);
>   }
>   if (pos < argTypes.length) {
>   System.arraycopy(argTypes, pos, newArgs, pos + 
> paramTypes.length, argTypes.length - pos);
>   }
>   return ofValidated(returnType, newArgs);
> }
> 
> 
> See also:
> https://github.com/openjdk/jdk/blob/230fac80f25e9608006c8928a8a7708bf13a818c/src/java.base/share/classes/java/util/ImmutableCollections.java#L186-L195

Nice catch! I conservatively just move the validation loop after to keep the 
arraycopying.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1594579344