On Wed, 15 May 2024 07:20:37 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:
>
> Use sb.repeat, consolidate
src/java.base/share/classes/java/lang/constant/ClassDesc.java line 341:
> 339: String desc = descriptorString();
> 340: int index = desc.lastIndexOf('/');
> 341: return (index == -1) ? "" : internalToBinary(desc.substring(1,
> index - 1));
Here it cuts the package name, for example
`ConstantDescs.CD_Integer.packageName()` returns `java.lan`
Suggestion:
return (index == -1) ? "" : internalToBinary(desc.substring(1, index));
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601328026
