Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v4]
On Thu, 9 May 2024 12:29:40 GMT, Adam Sotona wrote: >> I actually am not sure of the reason; it is indeed a tableswitch in >> bytecode. @cl4es might know it better? > > Generally I agree with the API changes and with the > `descriptor.isPrimitive()` test before requesting a descriptor string. > However I'm missing the point of the other changes, which only replace this > single `tableswitch` with something way more complicated, without clear > performance gain. The code I tested saw a decidedly larger speed-up, perhaps hinging on other optimizations such as being able to avoid the `s.isEmpty()` check (could maybe catch IIOBE) and not matching on `L` and `[` (the lookup table only needs to deal with primitives). As it looks here I agree the speed-up is negligible and not worth it, and since `TypeDescriptor.OfField` isn't a sealed hierarchy perhaps we need some redundant checks. - PR Review Comment: https://git.openjdk.org/jdk/pull/19109#discussion_r1595873311
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v4]
On Thu, 9 May 2024 12:07:22 GMT, Chen Liang wrote:
>> src/java.base/share/classes/java/lang/classfile/TypeKind.java line 139:
>>
>>> 137: case 'V' -> TypeKind.VoidType;
>>> 138: default -> throw new IllegalArgumentException("Bad type: "
>>> + s);
>>> 139: };
>>
>> This maps to a `tableswitch`, could you explain why a statically initialized
>> array addressed with a custom hash code suppose to be faster?
>> Thanks.
>
> I actually am not sure of the reason; it is indeed a tableswitch in bytecode.
> @cl4es might know it better?
Generally I agree with the API changes and with the `descriptor.isPrimitive()`
test before requesting a descriptor string.
However I'm missing the point of the other changes, which only replace this
single `tableswitch` with something way more complicated, without clear
performance gain.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19109#discussion_r1595381124
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v4]
On Thu, 9 May 2024 07:23:16 GMT, Adam Sotona wrote:
>> Chen Liang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Hash table, use fixed random seed
>
> src/java.base/share/classes/java/lang/classfile/TypeKind.java line 139:
>
>> 137: case 'V' -> TypeKind.VoidType;
>> 138: default -> throw new IllegalArgumentException("Bad type: "
>> + s);
>> 139: };
>
> This maps to a `tableswitch`, could you explain why a statically initialized
> array addressed with a custom hash code suppose to be faster?
> Thanks.
I actually am not sure of the reason; it is indeed a tableswitch in bytecode.
@cl4es might know it better?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19109#discussion_r1595360109
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v4]
On Tue, 7 May 2024 15:23:10 GMT, Chen Liang wrote:
>> A peek into TypeKind during the research for #19105 reveals that TypeKind
>> has a few issues:
>> 1. Name mismatch for `newarraycode` and `fromNewArrayCode`: Renamed both to
>> use "newarray code"
>> 2. `fromDescriptor` can throw IOOBE if the input string is empty: changed to
>> throw IAE and added tests.
>> 3. `from(Class)` can be slow due to descriptor computation: added benchmark,
>> will share result in next comment (as it may change with code changes).
>>
>> The first 2 changes involves API changes, and a CSR has been created.
>> Requesting @asotona for a review.
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Hash table, use fixed random seed
src/java.base/share/classes/java/lang/classfile/TypeKind.java line 139:
> 137: case 'V' -> TypeKind.VoidType;
> 138: default -> throw new IllegalArgumentException("Bad type: " +
> s);
> 139: };
This maps to a `tableswitch`, could you explain why a statically initialized
array addressed with a custom hash code suppose to be faster?
Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19109#discussion_r1595070662
Re: RFR: 8331744: java.lang.classfile.TypeKind improvements [v4]
> A peek into TypeKind during the research for #19105 reveals that TypeKind has > a few issues: > 1. Name mismatch for `newarraycode` and `fromNewArrayCode`: Renamed both to > use "newarray code" > 2. `fromDescriptor` can throw IOOBE if the input string is empty: changed to > throw IAE and added tests. > 3. `from(Class)` can be slow due to descriptor computation: added benchmark, > will share result in next comment (as it may change with code changes). > > The first 2 changes involves API changes, and a CSR has been created. > Requesting @asotona for a review. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Hash table, use fixed random seed - Changes: - all: https://git.openjdk.org/jdk/pull/19109/files - new: https://git.openjdk.org/jdk/pull/19109/files/adf1218c..9af30c65 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19109=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19109=02-03 Stats: 68 lines in 3 files changed: 53 ins; 8 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19109.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19109/head:pull/19109 PR: https://git.openjdk.org/jdk/pull/19109
