Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v3]
On Thu, 10 Mar 2022 08:52:17 GMT, Сергей Цыпанов wrote: >> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with >> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when >> called with vararg of size 0, 1, 2. >> >> In general replacement of `Arrays.asList()` with `List.of()` is dubious as >> the latter is null-hostile, however in some cases we are sure that arguments >> are non-null. Into this PR I've included the following cases (in addition to >> those where the argument is proved to be non-null at compile-time): >> - `MethodHandles.longestParameterList()` never returns null >> - parameter types are never null >> - interfaces used for proxy construction and returned from >> `Class.getInterfaces()` are never null >> - exceptions types of method signature are never null > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8282662: Revert dubious changes in MethodType There were changes with `Set.of()`, but I've reverted them as dubious. I'll rename the ticket and PR's title. As of the second question I didn't look namely for `Collections.addAll(set, elements)`, if I find any feasible for replacement with `Set.of()` then I'll add it. - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v3]
On Thu, 10 Mar 2022 08:52:17 GMT, Сергей Цыпанов wrote: >> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with >> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when >> called with vararg of size 0, 1, 2. >> >> In general replacement of `Arrays.asList()` with `List.of()` is dubious as >> the latter is null-hostile, however in some cases we are sure that arguments >> are non-null. Into this PR I've included the following cases (in addition to >> those where the argument is proved to be non-null at compile-time): >> - `MethodHandles.longestParameterList()` never returns null >> - parameter types are never null >> - interfaces used for proxy construction and returned from >> `Class.getInterfaces()` are never null >> - exceptions types of method signature are never null > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8282662: Revert dubious changes in MethodType Just curious, this issue asks to replace set constructions with `Set.of`, but there is no such code changes in this pull request. Is there any set creation patterns that aren't detected by your ide cleanups, such as `Collections.addAll(set, elements)` for creating hash set contents from array? - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v3]
> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with > smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when > called with vararg of size 0, 1, 2. > > In general replacement of `Arrays.asList()` with `List.of()` is dubious as > the latter is null-hostile, however in some cases we are sure that arguments > are non-null. Into this PR I've included the following cases (in addition to > those where the argument is proved to be non-null at compile-time): > - `MethodHandles.longestParameterList()` never returns null > - parameter types are never null > - interfaces used for proxy construction and returned from > `Class.getInterfaces()` are never null > - exceptions types of method signature are never null Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8282662: Revert dubious changes in MethodType - Changes: - all: https://git.openjdk.java.net/jdk/pull/7729/files - new: https://git.openjdk.java.net/jdk/pull/7729/files/5bbe8c4e..e765c42a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7729=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7729=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7729.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7729/head:pull/7729 PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
On Wed, 9 Mar 2022 16:09:01 GMT, liach wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8282662: Revert dubious changes > > src/java.base/share/classes/java/lang/invoke/MethodType.java line 910: > >> 908: if (skipPos > myLen || myLen - skipPos > fullLen) >> 909: return false; >> 910: List> myList = List.of(ptypes); > > imo should revert this one together with that proxy parameter one Reverted - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
On Wed, 9 Mar 2022 08:35:45 GMT, Сергей Цыпанов wrote: >> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with >> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when >> called with vararg of size 0, 1, 2. >> >> In general replacement of `Arrays.asList()` with `List.of()` is dubious as >> the latter is null-hostile, however in some cases we are sure that arguments >> are non-null. Into this PR I've included the following cases (in addition to >> those where the argument is proved to be non-null at compile-time): >> - `MethodHandles.longestParameterList()` never returns null >> - parameter types are never null >> - interfaces used for proxy construction and returned from >> `Class.getInterfaces()` are never null >> - exceptions types of method signature are never null > > Сергей Цыпанов has updated the pull request incrementally with one additional > commit since the last revision: > > 8282662: Revert dubious changes src/java.base/share/classes/java/lang/invoke/MethodType.java line 910: > 908: if (skipPos > myLen || myLen - skipPos > fullLen) > 909: return false; > 910: List> myList = List.of(ptypes); imo should revert this one together with that proxy parameter one - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
On Wed, 9 Mar 2022 09:37:30 GMT, Сергей Цыпанов wrote:
>> src/java.base/share/classes/java/nio/file/FileTreeIterator.java line 70:
>>
>>> 68: throws IOException
>>> 69: {
>>> 70: this.walker = new FileTreeWalker(List.of(options), maxDepth);
>>
>> Relates to https://bugs.openjdk.java.net/browse/JDK-8145048
>
> Should I keep it as is or revert along with the rest of dubious changes?
probably keep it. this can be updated in the patch for that bug.
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
On Tue, 8 Mar 2022 14:27:23 GMT, liach wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8282662: Revert dubious changes
>
> src/java.base/share/classes/java/nio/file/FileTreeIterator.java line 70:
>
>> 68: throws IOException
>> 69: {
>> 70: this.walker = new FileTreeWalker(List.of(options), maxDepth);
>
> Relates to https://bugs.openjdk.java.net/browse/JDK-8145048
Should I keep it as is or revert along with the rest of dubious changes?
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
On Tue, 8 Mar 2022 14:28:00 GMT, liach wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8282662: Revert dubious changes > > src/java.base/share/classes/sun/reflect/annotation/AnnotationSupport.java > line 79: > >> 77: >> containerBeforeContainee(annotations, annoClass); >> 78: >> 79: result.addAll((indirectFirst ? 0 : 1), List.of(indirect)); > > This `indirect` is most likely to be of size > 2 I've reverted this along with the rest of dubious changes - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption [v2]
> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with > smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when > called with vararg of size 0, 1, 2. > > In general replacement of `Arrays.asList()` with `List.of()` is dubious as > the latter is null-hostile, however in some cases we are sure that arguments > are non-null. Into this PR I've included the following cases (in addition to > those where the argument is proved to be non-null at compile-time): > - `MethodHandles.longestParameterList()` never returns null > - parameter types are never null > - interfaces used for proxy construction and returned from > `Class.getInterfaces()` are never null > - exceptions types of method signature are never null Сергей Цыпанов has updated the pull request incrementally with one additional commit since the last revision: 8282662: Revert dubious changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7729/files - new: https://git.openjdk.java.net/jdk/pull/7729/files/b31edfcd..5bbe8c4e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7729=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7729=00-01 Stats: 6 lines in 2 files changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7729.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7729/head:pull/7729 PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption
On Mon, 7 Mar 2022 15:11:50 GMT, Сергей Цыпанов wrote:
> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with
> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when
> called with vararg of size 0, 1, 2.
>
> In general replacement of `Arrays.asList()` with `List.of()` is dubious as
> the latter is null-hostile, however in some cases we are sure that arguments
> are non-null. Into this PR I've included the following cases (in addition to
> those where the argument is proved to be non-null at compile-time):
> - `MethodHandles.longestParameterList()` never returns null
> - parameter types are never null
> - interfaces used for proxy construction and returned from
> `Class.getInterfaces()` are never null
> - exceptions types of method signature are never null
Calling `Arrays.asList` vs `List.of` on a switch on the array length seems like
an overkill for such a simple thing. Imo the safest changes are where we know
the input array length is going to be 1 or 2.
src/java.base/share/classes/java/nio/file/FileTreeIterator.java line 70:
> 68: throws IOException
> 69: {
> 70: this.walker = new FileTreeWalker(List.of(options), maxDepth);
Relates to https://bugs.openjdk.java.net/browse/JDK-8145048
src/java.base/share/classes/sun/reflect/annotation/AnnotationSupport.java line
79:
> 77: containerBeforeContainee(annotations,
> annoClass);
> 78:
> 79: result.addAll((indirectFirst ? 0 : 1), List.of(indirect));
This `indirect` is most likely to be of size > 2
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption
On Mon, 7 Mar 2022 16:06:44 GMT, Claes Redestad wrote: > Notice list.of will have the downside of copying the input array when the > size is not small while arrays aslist does not. Is the tradeoff worth it? Good point, I see risky changes in this PR: - `ProxyGenerator` - `Proxy` - `MethodType` - `FileTreeIterator` As of `ProxyGenerator.ProxyMethod` at start-up time of my Spring Boot -based application there are 2696 invocations of `ProxyGenerator.ProxyMethod.generateMethod()` and 0 of them was for `exceptionTypes.length > 2`. This is expectable as the majority of the methods declare either 0, or 1-2 exceptions. As of `Proxy.getProxyConstructor()` in my application I have 1417 invocations of `List.of()` and 599 of them are with `intfsArray.length > 2`. There were no invocations of `Proxy.defaultMethodHandle()` so I have no statistics. For `MethodType` there were 0 invocations, and I have no statistics, but as of my experience most of the methods have 0-2 parameters. In case of `FileTreeIterator` incoming vararg is always of length either 0 or 1, as `FileVisitOption` enum has only one item. So I suggest to apply something like `intfsArray.length < 2 ? List.of(intfsArray) : Arrays.asList(intfsArray)` keeping the rest of the code as is. What do you think? - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption
On Mon, 7 Mar 2022 15:54:02 GMT, liach wrote: > Notice list.of will have the downside of copying the input array when the > size is not small while arrays aslist does not. Is the tradeoff worth it? A good observation. In a couple of these places we could probably use `JavaUtilCollectionAccess.listFromTrustedArray` to avoid such copies. - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption
On Mon, 7 Mar 2022 15:11:50 GMT, Сергей Цыпанов wrote: > `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with > smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when > called with vararg of size 0, 1, 2. > > In general replacement of `Arrays.asList()` with `List.of()` is dubious as > the latter is null-hostile, however in some cases we are sure that arguments > are non-null. Into this PR I've included the following cases (in addition to > those where the argument is proved to be non-null at compile-time): > - `MethodHandles.longestParameterList()` never returns null > - parameter types are never null > - interfaces used for proxy construction and returned from > `Class.getInterfaces()` are never null > - exceptions types of method signature are never null Notice list.of will have the downside of copying the input array when the size is not small while arrays aslist does not. Is the tradeoff worth it? - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption
On Mon, 7 Mar 2022 15:11:50 GMT, Сергей Цыпанов wrote: > `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with > smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when > called with vararg of size 0, 1, 2. > > In general replacement of `Arrays.asList()` with `List.of()` is dubious as > the latter is null-hostile, however in some cases we are sure that arguments > are non-null. Into this PR I've included the following cases (in addition to > those where the argument is proved to be non-null at compile-time): > - `MethodHandles.longestParameterList()` never returns null > - parameter types are never null > - interfaces used for proxy construction and returned from > `Class.getInterfaces()` are never null > - exceptions types of method signature are never null There's also another difference: they have different serial forms. From my cursory inspection it doesn't look like the returned list are put in serializable fields but it could be good to double check it. - PR: https://git.openjdk.java.net/jdk/pull/7729
