Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]
On Wed, 15 May 2024 07:20:37 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: > > Use sb.repeat, consolidate src/java.base/share/classes/java/lang/constant/ClassDesc.java line 190: > 188: MAX_ARRAY_TYPE_DESC_DIMENSIONS + " dimensions"); > 189: } > 190: String newDesc = new StringBuilder(desc.length() + > 1).append('[').append(desc).toString(); This could use [`String::concat(String)`], which uses the same underlying code path as the `invokedynamic`‑based `String` concatenation: Suggestion: String newDesc = "[".concat(desc); [`String::concat(String)`]: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/String.html#concat(java.lang.String) - PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601423761
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]
On Wed, 15 May 2024 09:51:13 GMT, Adam Sotona wrote: >> 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)); Thanks! Verified there were tests covering this. - PR Review Comment: https://git.openjdk.org/jdk/pull/19105#discussion_r1601383209
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]
On Wed, 15 May 2024 07:20:37 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: > > 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
Re: RFR: 8331724: Refactor j.l.constant implementation to internal package [v10]
> 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: Use sb.repeat, consolidate - Changes: - all: https://git.openjdk.org/jdk/pull/19105/files - new: https://git.openjdk.org/jdk/pull/19105/files/6d5df18e..b0aca160 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19105&range=08-09 Stats: 8 lines in 1 file changed: 1 ins; 5 del; 2 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