Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Fri, 3 Jun 2022 16:48:11 GMT, Jorn Vernee wrote: >> I'm not so sure. >> >> First of all we're no worse than before with the defensive copying here. >> Second of an optimizing compiler might theoretically be able to see that the >> array we get from the toArray is always fresh and not escaping anywhere in >> any well-behaved collection, so the clone could be elided. But if not then >> both toArray and clone are intrinsified operations that are heavily >> optimized and pretty fast even when interpreting, so emulating it with an >> external loop might end up taking more time even at peak. While likely >> taking longer to reach that peak. Using dumb, shared and common code >> (especially things that get JITted early anyhow) is nice in areas that see >> most action during startup/warmup. > > Ok, please keep it the way it is in your current patch then. Correction: `toArray` isn't intrinsified, but common `List` implementations are likely to implement it with a `System.arraycopy`, which is. - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Fri, 3 Jun 2022 16:43:37 GMT, Claes Redestad wrote: >> The same could be done for the public `dropArgumentsToMatch` I think. > > I'm not so sure. > > First of all we're no worse than before with the defensive copying here. > Second of an optimizing compiler might theoretically be able to see that the > array we get from the toArray is always fresh and not escaping anywhere in > any well-behaved collection, so the clone could be elided. But if not then > both toArray and clone are intrinsified operations that are heavily optimized > and pretty fast even when interpreting, so emulating it with an external loop > might end up taking more time even at peak. While likely taking longer to > reach that peak. Using dumb, shared and common code (especially things that > get JITted early anyhow) is nice in areas that see most action during > startup/warmup. Ok, please keep it the way it is in your current patch then. - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Fri, 3 Jun 2022 15:06:48 GMT, Jorn Vernee wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review comments, eagerly convert sooner in tryFinally
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5269:
>
>> 5267: }
>> 5268:
>> 5269: static MethodHandle dropArguments(MethodHandle target, int pos,
>> Class[] valueTypes, boolean trusted) {
>
> Having a boolean that flips the behaviour makes it harder to know what that
> `true` or `false` literal at the call site does. I'd suggest splitting this
> into `dropArgumentsTrusted`, which doesn't clone the array, and
> `dropArguments` which always makes a copy (and the latter calls former). WDYT?
Yeah, that sounds better.
-
PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Fri, 3 Jun 2022 15:18:52 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5266:
>>
>>> 5264: */
>>> 5265: public static MethodHandle dropArguments(MethodHandle target, int
>>> pos, List> valueTypes) {
>>> 5266: return dropArguments(target, pos, valueTypes.toArray(new
>>> Class[0]), false);
>>
>> A bit unfortunate that we can't trust this `toArray` to do a copy. I was
>> going to suggest Stream, but it has the same issue (someone might have a
>> custom stream implementation).
>>
>> I do think a manual copy of the array is possible by having a loop though:
>> Suggestion:
>>
>> Class[] ptypes = new Class[valueTypes.size()];
>> for (int i = 0; i < ptypes.length; i++) {
>> ptypes[i] = valueTypes.get(i);
>> }
>> return dropArguments(target, pos, ptypes, false);
>>
>>
>> (or, maybe extract such a loop to a helper method for clarity).
>
> The same could be done for the public `dropArgumentsToMatch` I think.
I'm not so sure.
First of all we're no worse than before with the defensive copying here. Second
of an optimizing compiler might theoretically be able to see that the array we
get from the toArray is always fresh and not escaping anywhere in any
well-behaved collection, so the clone could be elided. But if not then both
toArray and clone are intrinsified operations that are heavily optimized and
pretty fast even when interpreting, so emulating it with an external loop might
end up taking more time even at peak. While likely taking longer to reach that
peak. Using dumb, shared and common code (especially things that get JITted
early anyhow) is nice in areas that see most action during startup/warmup.
-
PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Fri, 3 Jun 2022 15:09:24 GMT, Jorn Vernee wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Review comments, eagerly convert sooner in tryFinally
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5266:
>
>> 5264: */
>> 5265: public static MethodHandle dropArguments(MethodHandle target, int
>> pos, List> valueTypes) {
>> 5266: return dropArguments(target, pos, valueTypes.toArray(new
>> Class[0]), false);
>
> A bit unfortunate that we can't trust this `toArray` to do a copy. I was
> going to suggest Stream, but it has the same issue (someone might have a
> custom stream implementation).
>
> I do think a manual copy of the array is possible by having a loop though:
> Suggestion:
>
> Class[] ptypes = new Class[valueTypes.size()];
> for (int i = 0; i < ptypes.length; i++) {
> ptypes[i] = valueTypes.get(i);
> }
> return dropArguments(target, pos, ptypes, false);
>
>
> (or, maybe extract such a loop to a helper method for clarity).
The same could be done for the public `dropArgumentsToMatch` I think.
-
PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Fri, 27 May 2022 19:52:30 GMT, Claes Redestad wrote:
>> In preparation of #8855 this PR refactors the conversions from `List` to
>> array and array to `List`, reducing the number of conversions when calling
>> `MethodHandles.dropArguments` in particular. This remove about ~5% of
>> allocations on the `StringConcatFactoryBootstraps` microbenchmark.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review comments, eagerly convert sooner in tryFinally
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5266:
> 5264: */
> 5265: public static MethodHandle dropArguments(MethodHandle target, int
> pos, List> valueTypes) {
> 5266: return dropArguments(target, pos, valueTypes.toArray(new
> Class[0]), false);
A bit unfortunate that we can't trust this `toArray` to do a copy. I was going
to suggest Stream, but it has the same issue (someone might have a custom
stream implementation).
I do think a manual copy of the array is possible by having a loop though:
Suggestion:
Class[] ptypes = new Class[valueTypes.size()];
for (int i = 0; i < ptypes.length; i++) {
ptypes[i] = valueTypes.get(i);
}
return dropArguments(target, pos, ptypes, false);
(or, maybe extract such a loop to a helper method for clarity).
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5269:
> 5267: }
> 5268:
> 5269: static MethodHandle dropArguments(MethodHandle target, int pos,
> Class[] valueTypes, boolean trusted) {
Having a boolean that flips the behaviour makes it harder to know what that
`true` or `false` literal at the call site does. I'd suggest splitting this
into `dropArgumentsTrusted`, which doesn't clone the array, and `dropArguments`
which always makes a copy (and the latter calls former). WDYT?
-
PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Thu, 2 Jun 2022 15:19:44 GMT, Сергей Цыпанов wrote: >> The clone is needed - as the `List>` may be a custom implementation >> of List - so you cannot make any assumption on the concrete implementation >> of `toArray`. > > But we can make an assumption on the behavior of particular method and the > JavaDoc is clear about it. If a user passes an incorrect implementation of > the `List` interface then it's their own responsibility. We can't rule out malicious code holding the array reference it returns in toArray and updates that array to cause damage. - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Thu, 2 Jun 2022 14:25:55 GMT, Daniel Fuchs wrote: >> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5462: >> >>> 5460: Objects.requireNonNull(target); >>> 5461: Objects.requireNonNull(newTypes); >>> 5462: return dropArgumentsToMatch(target, skip, >>> newTypes.toArray(new Class[0]).clone(), pos, false); >> >> Do we really need to clone an array returned from `List.toArray()`? As far >> as I know from the JavaDoc of `List` if the passed array is not long enough >> to include all the items then the new array must be allocated. Here we >> always pass empty arrays, so the new ones are returned from `toArray()` >> method and we don't need `clone()`, right? > > The clone is needed - as the `List>` may be a custom implementation > of List - so you cannot make any assumption on the concrete implementation of > `toArray`. But we can make an assumption on the behavior of particular method and the JavaDoc is clear about it. If a user passes an incorrect implementation of the `List` interface then it's their own responsibility. - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Thu, 2 Jun 2022 13:59:50 GMT, Сергей Цыпанов wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Review comments, eagerly convert sooner in tryFinally > > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5462: > >> 5460: Objects.requireNonNull(target); >> 5461: Objects.requireNonNull(newTypes); >> 5462: return dropArgumentsToMatch(target, skip, newTypes.toArray(new >> Class[0]).clone(), pos, false); > > Do we really need to clone an array returned from `List.toArray()`? As far as > I know from the JavaDoc of `List` if the passed array is not long enough to > include all the items then the new array must be allocated. Here we always > pass empty arrays, so the new ones are returned from `toArray()` method and > we don't need `clone()`, right? The clone is needed - as the `List>` may be a custom implementation of List - so you cannot make any assumption on the concrete implementation of `toArray`. - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Fri, 27 May 2022 19:52:30 GMT, Claes Redestad wrote: >> In preparation of #8855 this PR refactors the conversions from `List` to >> array and array to `List`, reducing the number of conversions when calling >> `MethodHandles.dropArguments` in particular. This remove about ~5% of >> allocations on the `StringConcatFactoryBootstraps` microbenchmark. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Review comments, eagerly convert sooner in tryFinally src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5462: > 5460: Objects.requireNonNull(target); > 5461: Objects.requireNonNull(newTypes); > 5462: return dropArgumentsToMatch(target, skip, newTypes.toArray(new > Class[0]).clone(), pos, false); Do we really need to clone an array returned from `List.toArray()`? As far as I know from the JavaDoc of `List` if the passed array is not long enough to include all the items then the new array must be allocated. Here we always pass empty arrays, so the new ones are returned from `toArray()` method and we don't need `clone()`, right? - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
> In preparation of #8855 this PR refactors the conversions from `List` to > array and array to `List`, reducing the number of conversions when calling > `MethodHandles.dropArguments` in particular. This remove about ~5% of > allocations on the `StringConcatFactoryBootstraps` microbenchmark. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Review comments, eagerly convert sooner in tryFinally - Changes: - all: https://git.openjdk.java.net/jdk/pull/8923/files - new: https://git.openjdk.java.net/jdk/pull/8923/files/019e2ef8..15ff2125 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8923=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8923=00-01 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8923.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8923/head:pull/8923 PR: https://git.openjdk.java.net/jdk/pull/8923
