Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v3]
On Fri, 3 Jun 2022 17:05:19 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: > > Jorn review: Drop boolean, rename internal variant dropArgumentsTrusted Marked as reviewed by jvernee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v3]
> 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: Jorn review: Drop boolean, rename internal variant dropArgumentsTrusted - Changes: - all: https://git.openjdk.java.net/jdk/pull/8923/files - new: https://git.openjdk.java.net/jdk/pull/8923/files/15ff2125..c590f0dd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8923&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8923&range=01-02 Stats: 13 lines in 3 files changed: 0 ins; 3 del; 10 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
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&pr=8923&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8923&range=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
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles
On Fri, 27 May 2022 19:27:44 GMT, ExE Boss wrote: > If `parameterList` is too slow for `List.of` copies the backing parameter > types array, wouldn't calling > `JavaUtilCollectionAccess::listFromTrustedArray` a better alternative, as it > only allocates one wrapper and has better performance on `subList` than > `Arrays.copyOfRange`? Good observation that `MethodType::parameterList()` is a plausible candidate for `listFromTrustedArray`. That could be a good standalone and orthogonal RFE. I recall @rose00 musing about how he'd prefer it if `MethodType` and friends would have been implemented with `List.of`-style immutable lists rather than arrays. While I agree with him in principle we now have a mix of both worlds where we're converting between either representation haphazardly, making the implementation more complex in the process. It seems better for now to streamline the implementation with an early conversion to what we use internally (arrays) and stick with that. - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles
On Fri, 27 May 2022 16:08:03 GMT, liach wrote: > If `parameterList` is too slow for `List.of` copies the backing parameter > types array, wouldn't calling > `JavaUtilCollectionAccess::listFromTrustedArray` a better alternative, as it > only allocates one wrapper and has better performance on `subList` than > `Arrays.copyOfRange`? No, because `listFromTrustedArray` doesn’t allow invocation with array classes other than `Object[].class`: https://github.com/openjdk/jdk/blob/6520843f86f638fe4d1e5b3358fab5799daca654/src/java.base/share/classes/java/util/ImmutableCollections.java#L195-L222 - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles
On Fri, 27 May 2022 14:38:27 GMT, Claes Redestad 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]).clone(), true);
>>
>> Isn't this call to `clone()` unnecessary, as `valueTypes.toArray` should
>> either return the passed empty array, or a newly created array?
>
> It might be a bit too paranoid in this instance (since we don't keep the
> array around for long), but not cloning the result of calling `toArray` on an
> arbitrary and possibly adversary `List` could open up for TOCTOU race bugs /
> attacks. The existing code was being paranoid and copying and I don't want to
> weaken something that could have security implications without double- and
> triple-checking that it's safe to do so.
You can probably call the `dropArguments` with `false` for `trusted` instead.
-
PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles
On Fri, 27 May 2022 14:18:19 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. If `parameterList` is too slow for `List.of` copies the backing parameter types array, wouldn't calling `JavaUtilCollectionAccess::listFromTrustedArray` a better alternative, as it only allocates one wrapper and has better performance on `subList` than `Arrays.copyOfRange`? - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles
On Fri, 27 May 2022 14:18:19 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.
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]).clone(), true);
Isn't this call to `clone()` unnecessary, as `valueTypes.toArray` should either
return the passed empty array, or a newly created array?
-
PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles
On Fri, 27 May 2022 14:18:19 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. It might be a bit too paranoid in this instance (since we don't keep the array around for long), but not cloning the result of calling `toArray` on an arbitrary and possibly adversary `List` could open up for TOCTOU race bugs / attacks. The existing code was being paranoid and copying and I don't want to weaken something that could have security implications without double- and triple-checking that it's safe to do so. - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles
On Fri, 27 May 2022 14:18:19 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. Another datapoint is the `MethodHandlesDropArguments.interCreate` microbenchmark which explicitly tests `MethodHandles.dropArguments` (with a minimal argument list): Before: BenchmarkMode Cnt ScoreError Units MethodHandlesDropArguments.interCreate avgt 5 136.778 ? 1.265 ns/op MethodHandlesDropArguments.interCreate:?gc.alloc.rate.norm avgt 5 264.020 ? 0.021B/op Patch: BenchmarkMode Cnt Score Error Units MethodHandlesDropArguments.interCreate avgt 5 120.536 ? 3.133 ns/op MethodHandlesDropArguments.interCreate:?gc.alloc.rate.norm avgt 5 220.818 ? 41.309B/op - PR: https://git.openjdk.java.net/jdk/pull/8923
