On Wed, 8 May 2024 10:42:27 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.
>
> 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
