Re: RFR: 8300237: Minor improvements in MethodHandles [v5]
On Wed, 18 Jan 2023 07:32:58 GMT, Sergey Tsypanov wrote: >> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` >> when we don't need a copy >> - comparison of two lists can be done without `Stream.reduce()` > > Sergey Tsypanov has updated the pull request incrementally with one > additional commit since the last revision: > > Use comparingInt() Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v5]
On Wed, 18 Jan 2023 07:32:58 GMT, Sergey Tsypanov wrote: >> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` >> when we don't need a copy >> - comparison of two lists can be done without `Stream.reduce()` > > Sergey Tsypanov has updated the pull request incrementally with one > additional commit since the last revision: > > Use comparingInt() Looks good to me, i'm not an official reviewer but claes already review this PR so this is fine. - PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v5]
> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` > when we don't need a copy > - comparison of two lists can be done without `Stream.reduce()` Sergey Tsypanov has updated the pull request incrementally with one additional commit since the last revision: Use comparingInt() - Changes: - all: https://git.openjdk.org/jdk/pull/12025/files - new: https://git.openjdk.org/jdk/pull/12025/files/f3fc18dc..85662895 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12025=04 - incr: https://webrevs.openjdk.org/?repo=jdk=12025=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12025.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12025/head:pull/12025 PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v4]
On Tue, 17 Jan 2023 22:28:50 GMT, Claes Redestad wrote: >> Sergey Tsypanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Merge map() calls > > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6752: > >> 6750: .map(MethodHandle::type) >> 6751: .filter(t -> t.parameterCount() > skipSize) >> 6752: .max(Comparator.comparing(MethodType::parameterCount)) > > @forax suggested `Comparator.comparingInt` here, which may or may not help > avoid some boxing. Nice point, done! - PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v4]
On Tue, 17 Jan 2023 21:24:34 GMT, Sergey Tsypanov wrote: >> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` >> when we don't need a copy >> - comparison of two lists can be done without `Stream.reduce()` > > Sergey Tsypanov has updated the pull request incrementally with one > additional commit since the last revision: > > Merge map() calls LGTM src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6752: > 6750: .map(MethodHandle::type) > 6751: .filter(t -> t.parameterCount() > skipSize) > 6752: .max(Comparator.comparing(MethodType::parameterCount)) @forax suggested `Comparator.comparingInt` here, which may or may not help avoid some boxing. - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v4]
> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` > when we don't need a copy > - comparison of two lists can be done without `Stream.reduce()` Sergey Tsypanov has updated the pull request incrementally with one additional commit since the last revision: Merge map() calls - Changes: - all: https://git.openjdk.org/jdk/pull/12025/files - new: https://git.openjdk.org/jdk/pull/12025/files/cab00aee..f3fc18dc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12025=03 - incr: https://webrevs.openjdk.org/?repo=jdk=12025=02-03 Stats: 10 lines in 1 file changed: 7 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12025.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12025/head:pull/12025 PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v3]
On Tue, 17 Jan 2023 20:51:06 GMT, Rémi Forax wrote: >> Sergey Tsypanov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Polishing > > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 59: > >> 57: import java.nio.ByteOrder; >> 58: import java.security.ProtectionDomain; >> 59: import java.util.*; > > oops Reverted > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6747: > >> 6745: .max(Comparator.comparing(MethodType::parameterCount)) >> 6746: .map(MethodType::ptypes) >> 6747: .map(longest -> List.of(Arrays.copyOfRange(longest, >> skipSize, longest.length))) > > i think you can fuse these two map() calls Done! - PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v3]
On Tue, 17 Jan 2023 18:07:37 GMT, Sergey Tsypanov wrote: >> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` >> when we don't need a copy >> - comparison of two lists can be done without `Stream.reduce()` > > Sergey Tsypanov has updated the pull request incrementally with one > additional commit since the last revision: > > Polishing src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 59: > 57: import java.nio.ByteOrder; > 58: import java.security.ProtectionDomain; > 59: import java.util.*; oops src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6747: > 6745: .max(Comparator.comparing(MethodType::parameterCount)) > 6746: .map(MethodType::ptypes) > 6747: .map(longest -> List.of(Arrays.copyOfRange(longest, > skipSize, longest.length))) i think you can fuse these to map() calls - PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v3]
On Tue, 17 Jan 2023 15:30:12 GMT, Rémi Forax wrote: >> Precious little method handle use in lambda bootstrap since JDK 11. Though I >> agree with the sentiment - having fixed a number of bootstrap issues in the >> past - `MethodHandles` is a small step up the abstraction ladder and the >> code in particular already uses a number of method refs and lambdas. > > ok, two small changes, > - formatting: usually the method call in a stream are aligned with the '.' > at the beginning > ``` > stream >.filter(...) > .map(...) >``` >instead of at the end. > > - the reduce is a max(), > `max(Comparator.comparingInt(List::size))` @forax formatting is fixed. As of max() I think we can improve this even more by hoisting max() before calling ptypes() - PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v3]
> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` > when we don't need a copy > - comparison of two lists can be done without `Stream.reduce()` Sergey Tsypanov has updated the pull request incrementally with one additional commit since the last revision: Polishing - Changes: - all: https://git.openjdk.org/jdk/pull/12025/files - new: https://git.openjdk.org/jdk/pull/12025/files/6d1397c4..cab00aee Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12025=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12025=01-02 Stats: 14 lines in 1 file changed: 0 ins; 6 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/12025.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12025/head:pull/12025 PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v2]
On Tue, 17 Jan 2023 10:18:42 GMT, Claes Redestad wrote: >> Using lambdas inside MethodHandles is quite dangerous given that lambdas are >> initialized using method handles. It may work now because >> longuestParameterList() is not called when initializing a lambda but it may >> make any changes in the implementation of lambdas painfull in the future. > > Precious little method handle use in lambda bootstrap since JDK 11. Though I > agree with the sentiment - having fixed a number of bootstrap issues in the > past - `MethodHandles` is a small step up the abstraction ladder and the code > in particular already uses a number of method refs and lambdas. ok, two small changes, - formatting: usually the method call in a stream are aligned with the '.' at the beginning ``` stream .filter(...) .map(...) ``` instead of at the end. - the reduce is a max(), `max(Comparator.comparingInt(List::size))` - PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles [v2]
> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` > when we don't need a copy > - comparison of two lists can be done without `Stream.reduce()` Sergey Tsypanov has updated the pull request incrementally with one additional commit since the last revision: Simplify - Changes: - all: https://git.openjdk.org/jdk/pull/12025/files - new: https://git.openjdk.org/jdk/pull/12025/files/e9e0baa0..6d1397c4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12025=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12025=00-01 Stats: 6 lines in 1 file changed: 1 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12025.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12025/head:pull/12025 PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles
On Tue, 17 Jan 2023 10:11:49 GMT, Rémi Forax wrote:
>> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6754:
>>
>>> 6752: filter(t -> t.parameterCount() > skipSize).
>>> 6753: map(MethodType::ptypes).
>>> 6754: reduce((p, q) -> p.length >= q.length ? p :
>>> q).orElse(EMPTY);
>>
>> Could avoid the need to introduce `EMPTY` if you make the stream expression
>> return the longest list directly:
>>
>> reduce((p, q) -> {
>> var longest = (p.size() >= q.size()) ? p : q;
>> return List.of(Arrays.copyOfRange(longest, skipSize,
>> longest.size()); // checking isEmpty() is redundant here since we filter on
>> `t.parameterCount() > skipSize`
>> }).orElse(List.of());
>
> Using lambdas inside MethodHandles is quite dangerous given that lambdas are
> initialized using method handles. It may work now because
> longuestParameterList() is not called when initializing a lambda but it may
> make any changes in the implementation of lambdas painfull in the future.
Precious little method handle use in lambda bootstrap since JDK 11. Though I
agree with the sentiment - having fixed a number of bootstrap issues in the
past - `MethodHandles` is a small step up the abstraction ladder and the code
in particular already uses a number of method refs and lambdas.
-
PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles
On Tue, 17 Jan 2023 09:51:31 GMT, Claes Redestad wrote:
>> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()`
>> when we don't need a copy
>> - comparison of two lists can be done without `Stream.reduce()`
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6754:
>
>> 6752: filter(t -> t.parameterCount() > skipSize).
>> 6753: map(MethodType::ptypes).
>> 6754: reduce((p, q) -> p.length >= q.length ? p :
>> q).orElse(EMPTY);
>
> Could avoid the need to introduce `EMPTY` if you make the stream expression
> return the longest list directly:
>
> reduce((p, q) -> {
> var longest = (p.size() >= q.size()) ? p : q;
> return List.of(Arrays.copyOfRange(longest, skipSize,
> longest.size()); // checking isEmpty() is redundant here since we filter on
> `t.parameterCount() > skipSize`
> }).orElse(List.of());
Using lambdas inside MethodHandles is quite dangerous given that lambdas are
initialized using method handles. It may work now because
longuestParameterList() is not called when initializing a lambda but it may
make any changes in the implementation of lambdas painfull in the future.
-
PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles
On Tue, 17 Jan 2023 08:22:28 GMT, Sergey Tsypanov wrote: > - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()` > when we don't need a copy > - comparison of two lists can be done without `Stream.reduce()` Remove `EMPTY` (using my earlier suggestion or by simply inlining `new Class[0]`) and I think this looks OK if framed as a code cleanup. The affected code isn't very performance sensitive as it's only used in the setup for `MHs.loop` combinators, which are likely to be rare in practice. - Changes requested by redestad (Reviewer). PR: https://git.openjdk.org/jdk/pull/12025
Re: RFR: 8300237: Minor improvements in MethodHandles
On Tue, 17 Jan 2023 08:22:28 GMT, Sergey Tsypanov wrote:
> - `MethodType.ptypes()` can be used instead of `MethodType.parameterList()`
> when we don't need a copy
> - comparison of two lists can be done without `Stream.reduce()`
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 6754:
> 6752: filter(t -> t.parameterCount() > skipSize).
> 6753: map(MethodType::ptypes).
> 6754: reduce((p, q) -> p.length >= q.length ? p :
> q).orElse(EMPTY);
Could avoid the need to introduce `EMPTY` if you make the stream expression
return the longest list directly:
reduce((p, q) -> {
var longest = (p.size() >= q.size()) ? p : q;
return List.of(Arrays.copyOfRange(longest, skipSize,
longest.size()); // checking isEmpty() is redundant here since we filter on
`t.parameterCount() > skipSize`
}).orElse(List.of());
-
PR: https://git.openjdk.org/jdk/pull/12025
