Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Mon, 16 May 2022 15:29:38 GMT, Сергей Цыпанов wrote: >> Even in the no exceptions case, the `exceptionTypes` array still has to be >> allocated/copied by `Method.getExceptionTypes()`[^1] when the `ProxyMethod` >> constructor[^2] is invoked. >> >> So if anything, the `List.of(…)` call should be moved into the `ProxyMethod` >> constructor. >> And maybe the call to `Method.getExceptionTypes()` should be changed to >> `Method.getSharedExceptionTypes()`[^3]. >> >> [^1]: >> https://github.com/openjdk/jdk/blob/e765c42aa71a25a9c30f3409b8fdc6bda6f7b639/src/java.base/share/classes/java/lang/reflect/Method.java#L340-L343 >> [^2]: >> https://github.com/openjdk/jdk/blob/e765c42aa71a25a9c30f3409b8fdc6bda6f7b639/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java#L710-L714 >> [^3]: >> https://github.com/openjdk/jdk/blob/e765c42aa71a25a9c30f3409b8fdc6bda6f7b639/src/java.base/share/classes/java/lang/reflect/Method.java#L305-L308 > >> So if anything, the List.of(…) call should be moved into the ProxyMethod >> constructor. And maybe the call to Method.getExceptionTypes() should be >> changed to Method.getSharedExceptionTypes() > > Makes sense. Do you want me to do this within this PR (this would be a > deviation from ticket's target), or it's better to create another one? Ok, I've reverted `ProxyGenerator` and will include the changes into another PR - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Fri, 13 May 2022 13:36:52 GMT, ExE Boss wrote: > So if anything, the List.of(…) call should be moved into the ProxyMethod > constructor. And maybe the call to Method.getExceptionTypes() should be > changed to Method.getSharedExceptionTypes() Makes sense. Do you want me to do this within this PR (this would be a deviation from ticket's target), or it's better to create another one? - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Fri, 13 May 2022 12:19:25 GMT, Сергей Цыпанов wrote: >> Usually a method declares either no exception, or a couple of them. In the >> first case `List.of()` doesn't allocate, in the second it allocates an >> object with 1-2 fields but without an array, so `List.of()` is likely to be >> more memory-saving > > Usually a method declares either no exception, or a couple of them. In the > first case `List.of()` doesn't allocate, in the second it allocates an object > with 1-2 fields but without an array, so `List.of()` is likely to be more > memory-saving Even in the no exceptions case, the `exceptionTypes` array still has to be allocated/copied by `Method.getExceptionTypes()`[^1] when the `ProxyMethod` constructor[^2] is invoked. So if anything, the `List.of(…)` call should be moved into the `ProxyMethod` constructor. And maybe the call to `Method.getExceptionTypes()` should be changed to `Method.getSharedExceptionTypes()`[^3]. [^1]: https://github.com/openjdk/jdk/blob/e765c42aa71a25a9c30f3409b8fdc6bda6f7b639/src/java.base/share/classes/java/lang/reflect/Method.java#L340-L343 [^2]: https://github.com/openjdk/jdk/blob/e765c42aa71a25a9c30f3409b8fdc6bda6f7b639/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java#L710-L714 [^3]: https://github.com/openjdk/jdk/blob/e765c42aa71a25a9c30f3409b8fdc6bda6f7b639/src/java.base/share/classes/java/lang/reflect/Method.java#L305-L308 - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Fri, 13 May 2022 11:14:29 GMT, ExE Boss wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8282662: Revert dubious changes in MethodType > > src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 727: > >> 725: MethodVisitor mv = cw.visitMethod(accessFlags, >> 726: method.getName(), desc, null, >> 727: typeNames(List.of(exceptionTypes))); > > Since `exceptionTypes` is an array, it might be better to keep using > `Arrays.asList`, or add an overload for `typeNames` that works on class > arrays. Usually a method declares either no exception, or a couple of them. In the first case `List.of()` doesn't allocate, in the second it allocates an object with 1-2 fields but without an array, so `List.of()` is likely to be more memory-saving - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Fri, 13 May 2022 12:19:08 GMT, Сергей Цыпанов wrote: >> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 727: >> >>> 725: MethodVisitor mv = cw.visitMethod(accessFlags, >>> 726: method.getName(), desc, null, >>> 727: typeNames(List.of(exceptionTypes))); >> >> Since `exceptionTypes` is an array, it might be better to keep using >> `Arrays.asList`, or add an overload for `typeNames` that works on class >> arrays. > > Usually a method declares either no exception, or a couple of them. In the > first case `List.of()` doesn't allocate, in the second it allocates an object > with 1-2 fields but without an array, so `List.of()` is likely to be more > memory-saving Usually a method declares either no exception, or a couple of them. In the first case `List.of()` doesn't allocate, in the second it allocates an object with 1-2 fields but without an array, so `List.of()` is likely to be more memory-saving - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method 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 src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 727: > 725: MethodVisitor mv = cw.visitMethod(accessFlags, > 726: method.getName(), desc, null, > 727: typeNames(List.of(exceptionTypes))); Since `exceptionTypes` is an array, it might be better to keep using `Arrays.asList`, or add an overload for `typeNames` that works on class arrays. - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Thu, 12 May 2022 14:14:38 GMT, Weijun Wang wrote:
>> Сергей Цыпанов has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8282662: Revert dubious changes in MethodType
>
> src/java.base/share/classes/sun/security/validator/EndEntityChecker.java line
> 119:
>
>> 117: // TLS key exchange algorithms requiring keyEncipherment key usage
>> 118: private static final Collection KU_SERVER_ENCRYPTION =
>> 119: Arrays.asList("RSA");
>
> I understand that you haven't modified the `KU_SERVER_KEY_AGREEMENT` on line
> 122 because it has 4 elements. However, these 2 nearby lines using different
> styles look a little strange to me. Why restrict the number to 0, 1, and 2?
> If we have defined methods with up to 9 arguments, does that mean the new
> type is suitable for 9 elements?
The reason is that for the cases of length 0, 1, 2 we don't create underlying
array as we do for larger number of elements. In other words when we replace
`Arrays.asList()` with `List.of()` we win only for 0, 1 and 2 elements.
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method 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
src/java.base/share/classes/sun/security/validator/EndEntityChecker.java line
119:
> 117: // TLS key exchange algorithms requiring keyEncipherment key usage
> 118: private static final Collection KU_SERVER_ENCRYPTION =
> 119: Arrays.asList("RSA");
I understand that you haven't modified the `KU_SERVER_KEY_AGREEMENT` on line
122 because it has 4 elements. However, these 2 nearby lines using different
styles look a little strange to me. Why restrict the number to 0, 1, and 2? If
we have defined methods with up to 9 arguments, does that mean the new type is
suitable for 9 elements?
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method 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 Let's wait a bit - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Sat, 26 Mar 2022 16:35:14 GMT, liach wrote: >> Сергей Цыпанов 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? @liach I've found a couple of places: - `AsynchronousFileChannel.open()` - `FileChannel.open()` - `DecimalStyle.getAvailableLocales()` They all either rely on user input, i.e. might throw NPE or IAE for nulls or duplicates respectively, or return the set, which is currently modifiable `HashSet`. I think the possible win isn't worth the effort. - PR: https://git.openjdk.java.net/jdk/pull/7729
