Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
On Thu, 23 May 2024 09:46:51 GMT, Jan Lahoda wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add type switch to HelloClasslist > > src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 53: > >> 51: import java.lang.classfile.instruction.SwitchCase; >> 52: >> 53: import jdk.internal.constant.MethodTypeDescImpl; > > Nit - this import seems to be unused, and neither seem to be the changes to > `MethodTypeDescImpl`. Is there some use missing? (OTOH, I like the > `StaticHolders` for the stuff that relates to the enum-switch special case, > for the time being.) Oops, some leftovers from the optimizations I reverted. Fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19307#discussion_r1611478359
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
On Tue, 21 May 2024 09:01:32 GMT, Claes Redestad wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes moving some seldom used `findStatic` >> calls to a holder. All in all this means a reduction by 22M cycles to >> bootstrap a trivial switch expression on my M1. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add type switch to HelloClasslist Seems goo to me, with one nit related to `MethodTypeDescImpl`. src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 53: > 51: import java.lang.classfile.instruction.SwitchCase; > 52: > 53: import jdk.internal.constant.MethodTypeDescImpl; Nit - this import seems to be unused, and neither seem to be the changes to `MethodTypeDescImpl`. Is there some use missing? (OTOH, I like the `StaticHolders` for the stuff that relates to the enum-switch special case, for the time being.) - PR Review: https://git.openjdk.org/jdk/pull/19307#pullrequestreview-2073448995 PR Review Comment: https://git.openjdk.org/jdk/pull/19307#discussion_r1611368747
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
On Tue, 21 May 2024 09:01:32 GMT, Claes Redestad wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes moving some seldom used `findStatic` >> calls to a holder. All in all this means a reduction by 22M cycles to >> bootstrap a trivial switch expression on my M1. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add type switch to HelloClasslist Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19307#pullrequestreview-2068663088
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
On Tue, 21 May 2024 09:01:32 GMT, Claes Redestad wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes moving some seldom used `findStatic` >> calls to a holder. All in all this means a reduction by 22M cycles to >> bootstrap a trivial switch expression on my M1. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add type switch to HelloClasslist [f04d78e](https://github.com/openjdk/jdk/pull/19307/commits/f04d78ea53ed0074026311f82eb0d4eafee3438d) passed tier1-3 testing - PR Comment: https://git.openjdk.org/jdk/pull/19307#issuecomment-2122519106
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
On Tue, 21 May 2024 09:01:32 GMT, Claes Redestad wrote: >> We can fold the call to `Objects.checkIndex` into the code generated in >> generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. >> This loads 9 less classes (of which 8 generated LFs and Species classes) on >> a minimal test, while being neutral on a throughput sanity test: >> >> >> Name Cnt Base Error Test Error Unit Change >> SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = >> 0,800 ) >> * = significant >> ``` >> >> A few additional optimizations includes moving some seldom used `findStatic` >> calls to a holder. All in all this means a reduction by 22M cycles to >> bootstrap a trivial switch expression on my M1. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add type switch to HelloClasslist By adding a trivial type switch to `HelloClasslist` we archive a set of classes and avoid some runtime class generation. This grows the default CDS archive by approximately 1.2Mb by pulling in a large number of Classfile API classes, which is bound to happen soon anyway. On the SwitchSanity test, bootstrap cost drops by another 35M cycles: Wall clock:51.0 ms/op .taskclock:63.5 ms/op .cycles: 220064872 average cycles elapsed .instructions: 544981276 average instructions retired - PR Comment: https://git.openjdk.org/jdk/pull/19307#issuecomment-2122136668
Re: RFR: 8332528: Generate code in SwitchBootstraps.generateTypeSwitch that require fewer adaptations [v3]
> We can fold the call to `Objects.checkIndex` into the code generated in > generateTypeSwitchSkeleton instead of doing so by filtering the MH argument. > This loads 9 less classes (of which 8 generated LFs and Species classes) on a > minimal test, while being neutral on a throughput sanity test: > > > Name Cnt Base Error Test Error Unit Change > SwitchSanity.switchSum 15 8,162 ± 0,117 8,152 ± 0,131 ns/op 1,00x (p = > 0,800 ) > * = significant > ``` > > A few additional optimizations includes moving some seldom used `findStatic` > calls to a holder. All in all this means a reduction by 22M cycles to > bootstrap a trivial switch expression on my M1. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add type switch to HelloClasslist - Changes: - all: https://git.openjdk.org/jdk/pull/19307/files - new: https://git.openjdk.org/jdk/pull/19307/files/c212a3d5..f04d78ea Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19307=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19307=01-02 Stats: 11 lines in 1 file changed: 11 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19307.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19307/head:pull/19307 PR: https://git.openjdk.org/jdk/pull/19307
