Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v5]
On Fri, 27 May 2022 00:16:00 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: > > Doesn't hurt to be more specific Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v5]
> 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: Doesn't hurt to be more specific - Changes: - all: https://git.openjdk.java.net/jdk/pull/8801/files - new: https://git.openjdk.java.net/jdk/pull/8801/files/6d171268..6aa89eff Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8801=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8801=03-04 Stats: 11 lines in 1 file changed: 1 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
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v4]
On Thu, 26 May 2022 23:42:27 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: > > Make primitive type info more reader friendly src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 979: > 977: unwrapMethodDesc = "()" + baseTypeString; > 978: this.loadOpcode = loadOpcode; > 979: this.returnOpcode = loadOpcode - ILOAD + IRETURN; This could do it. It would be more explicit to take the return opcode as an argument to the constructor. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 23:35:10 GMT, liach wrote: >> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: >> >>> 967: // single-char BaseType descriptor (see JVMS section 4.3.2) >>> 968: String baseTypeString = wrapper.basicTypeString(); >>> 969: wrapperClassName = >>> dotToSlash(wrapper.wrapperType().getName()); >> >> Suggestion: >> >> wrapperClassName = wrapper.wrapperType().descriptorString(); >> >> >> It may worth to replace similar use of `dotToSlash(c.getName())` pattern. > > Unfortunately, we want an internal name (`xxx/Abc`) than a field descriptor > (`Lxxx/Abc;`). But we can use descriptor string for the valueOf descriptor > construction. ah, you're right. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 22:55:02 GMT, Mandy Chung wrote: >> liach has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains three additional commits >> since the last revision: >> >> - Merge branch 'master' into proxy-primitivetypeinfo >> - Convert PrimitiveTypeInfo to an enum >> - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo > > Have you considered including the opcode for load and return instruction > instead of `opcodeOffset`? Hi @mlchung Mandy, I've changed the offset to seperate load and return opcodes, which is more in-line with how other fields in `PrimitiveTypeInfo` are used. In addition, I specified that the `wrapperClassName` is an internal name for clarification. Would you mind review this again, and review #8800, which avoids eager class initialization for parameter types used by proxy? - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 22:51:39 GMT, Mandy Chung wrote: >> liach has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains three additional commits >> since the last revision: >> >> - Merge branch 'master' into proxy-primitivetypeinfo >> - Convert PrimitiveTypeInfo to an enum >> - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo > > src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: > >> 967: // single-char BaseType descriptor (see JVMS section 4.3.2) >> 968: String baseTypeString = wrapper.basicTypeString(); >> 969: wrapperClassName = >> dotToSlash(wrapper.wrapperType().getName()); > > Suggestion: > > wrapperClassName = wrapper.wrapperType().descriptorString(); > > > It may worth to replace similar use of `dotToSlash(c.getName())` pattern. Unfortunately, we want an internal name (`xxx/Abc`) than a field descriptor (`Lxxx/Abc;`). But we can use descriptor string for the valueOf descriptor construction. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v4]
> 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: Make primitive type info more reader friendly - Changes: - all: https://git.openjdk.java.net/jdk/pull/8801/files - new: https://git.openjdk.java.net/jdk/pull/8801/files/96c0835e..6d171268 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8801=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8801=02-03 Stats: 26 lines in 1 file changed: 7 ins; 1 del; 18 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
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 20:55:53 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 with a new target base due to a merge or a > rebase. The incremental webrev excludes the unrelated changes brought in by > the merge/rebase. The pull request contains three additional commits since > the last revision: > > - Merge branch 'master' into proxy-primitivetypeinfo > - Convert PrimitiveTypeInfo to an enum > - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo Have you considered including the opcode for load and return instruction instead of `opcodeOffset`? src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: > 967: // single-char BaseType descriptor (see JVMS section 4.3.2) > 968: String baseTypeString = wrapper.basicTypeString(); > 969: wrapperClassName = > dotToSlash(wrapper.wrapperType().getName()); Suggestion: wrapperClassName = wrapper.wrapperType().descriptorString(); It may worth to replace similar use of `dotToSlash(c.getName())` pattern. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 986: > 984: if (cl == float.class) return FLOAT; > 985: if (cl == double.class) return DOUBLE; > 986: throw new AssertionError(); Suggestion: throw new AssertionError(cl); - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' into proxy-primitivetypeinfo - Convert PrimitiveTypeInfo to an enum - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo - Changes: - all: https://git.openjdk.java.net/jdk/pull/8801/files - new: https://git.openjdk.java.net/jdk/pull/8801/files/be9987bb..96c0835e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8801=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8801=01-02 Stats: 37306 lines in 626 files changed: 8637 ins; 27109 del; 1560 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
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
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo
On Fri, 20 May 2022 04:55:37 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)
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?
-
PR: https://git.openjdk.java.net/jdk/pull/8801
RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo
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) - Commit messages: - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo Changes: https://git.openjdk.java.net/jdk/pull/8801/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8801=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287064 Stats: 79 lines in 1 file changed: 14 ins; 41 del; 24 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
