Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]
On Fri, 20 May 2022 22:18:42 GMT, liach wrote: >> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace >> the hash map with a simple lookup, similar to what's done in >> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Convert PrimitiveTypeInfo to an enum I would prefer to look up in a `Map.ofEntries` than to switch on string class names, which is both faster and clearer. The even faster heuristic of using perfect hash table has already been proven slower than the list of if statements in JDK-8284880, referred to in the pull request description. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]
On Fri, 20 May 2022 22:18:42 GMT, liach wrote:
>> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace
>> the hash map with a simple lookup, similar to what's done in
>> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)
>
> liach has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Convert PrimitiveTypeInfo to an enum
> > Did you consider switch (class) {...} in PrimitiveTypeInfo.get?
>
> I don't think we can switch on class instances yet. Even with preview
> enabled, best I can get is (for type incompatibility of `Class` and
> `Class`, etc.)
>
> ```java
> return switch (cl) {
> case Class e && e == int.class -> 1;
> case Class e && e == long.class -> 2;
> case Class e && e == boolean.class -> 3;
> case Class e && e == short.class -> 4;
> case Class e && e == byte.class -> 5;
> case Class e && e == char.class -> 6;
> case Class e && e == float.class -> 7;
> case Class e && e == double.class -> 8;
> default -> 0;
> };
> ```
>
> to avoid type incompatibility; this is in turn implemented by a bootstrap
> method and a loop, which is of course obviously much slower.
Not necessarily recommending this coding idiom, but if you screened on
Class.isPrimitive() being true (taking care to avoid void.class), you could
switch on the string presentations on the class such as getName or
getSimpleName.
-
PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]
On Tue, 24 May 2022 17:26:52 GMT, Roger Riggs wrote:
> Did you consider switch (class) {...} in PrimitiveTypeInfo.get?
I don't think we can switch on class instances yet. Even with preview enabled,
best I can get is (for type incompatibility of `Class` and `Class`,
etc.)
return switch (cl) {
case Class e && e == int.class -> 1;
case Class e && e == long.class -> 2;
case Class e && e == boolean.class -> 3;
case Class e && e == short.class -> 4;
case Class e && e == byte.class -> 5;
case Class e && e == char.class -> 6;
case Class e && e == float.class -> 7;
case Class e && e == double.class -> 8;
default -> 0;
};
to avoid type incompatibility; this is in turn implemented by a bootstrap
method and a loop, which is of course obviously much slower.
-
PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]
On Fri, 20 May 2022 22:18:42 GMT, liach wrote:
>> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace
>> the hash map with a simple lookup, similar to what's done in
>> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242)
>
> liach has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Convert PrimitiveTypeInfo to an enum
Looks good.
Did you consider switch (class) {...} in PrimitiveTypeInfo.get?
The `if` cascade may be quicker given the expected distribution of lookups.
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]
On Fri, 20 May 2022 20:02:28 GMT, Roger Riggs wrote:
>> liach has updated the pull request incrementally with one additional commit
>> since the last revision:
>>
>> Convert PrimitiveTypeInfo to an enum
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 932:
>
>> 930: */
>> 931: private static final class PrimitiveTypeInfo {
>> 932: private static final PrimitiveTypeInfo BYTE = new
>> PrimitiveTypeInfo(byte.class, 0);
>
> Can this be `private enum PrimitiveTypeInfo...` or perhaps a record class?
Thanks. I've converted it into an enum following the suit of
`sun.invoke.util.Wrapper`. I think an enum fits better here as we don't need
the canonical constructor and object methods of a record.
-
PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v2]
> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the > hash map with a simple lookup, similar to what's done in > [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) liach has updated the pull request incrementally with one additional commit since the last revision: Convert PrimitiveTypeInfo to an enum - Changes: - all: https://git.openjdk.java.net/jdk/pull/8801/files - new: https://git.openjdk.java.net/jdk/pull/8801/files/16f3de8f..be9987bb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8801=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8801=00-01 Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/8801.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8801/head:pull/8801 PR: https://git.openjdk.java.net/jdk/pull/8801
