Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
> Currently, the two subclasses of `java.util.EnumSet` optimize bulk operations > when the argument is also a `EnumSet`, but there is no such optimization for > wrapper sets (returned by `Collections.unmodifiableSet`, > `Collections.synchronizedSet`, etc.) and immutable sets (returned by `Set.of` > methods) of `Enum`s. > > This PR introduces optimization classes for these situations. No public APIs > are changed. Tingjun Yuan has updated the pull request incrementally with one additional commit since the last revision: Set.copyOf: need defensive copy - Changes: - all: https://git.openjdk.org/jdk/pull/12498/files - new: https://git.openjdk.org/jdk/pull/12498/files/b9699bfe..0e0b2bf1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12498.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12498/head:pull/12498 PR: https://git.openjdk.org/jdk/pull/12498
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
Hello Stuart Marks, would you be able to take a look at this patch, improving performance of enum-only immutable collections, as you are the primary engineer in this area? On Mon, Feb 20, 2023 at 9:40 PM Tingjun Yuan wrote: > > > Currently, the two subclasses of `java.util.EnumSet` optimize bulk > > operations when the argument is also a `EnumSet`, but there is no such > > optimization for wrapper sets (returned by `Collections.unmodifiableSet`, > > `Collections.synchronizedSet`, etc.) and immutable sets (returned by > > `Set.of` methods) of `Enum`s. > > > > This PR introduces optimization classes for these situations. No public > > APIs are changed. > > Tingjun Yuan has updated the pull request incrementally with one additional > commit since the last revision: > > Set.copyOf: need defensive copy > > - > > Changes: > - all: https://git.openjdk.org/jdk/pull/12498/files > - new: https://git.openjdk.org/jdk/pull/12498/files/b9699bfe..0e0b2bf1 > > Webrevs: > - full: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=04 > - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12498&range=03-04 > > Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod > Patch: https://git.openjdk.org/jdk/pull/12498.diff > Fetch: git fetch https://git.openjdk.org/jdk pull/12498/head:pull/12498 > > PR: https://git.openjdk.org/jdk/pull/12498
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote: >> Currently, the two subclasses of `java.util.EnumSet` optimize bulk >> operations when the argument is also a `EnumSet`, but there is no such >> optimization for wrapper sets (returned by `Collections.unmodifiableSet`, >> `Collections.synchronizedSet`, etc.) and immutable sets (returned by >> `Set.of` methods) of `Enum`s. >> >> This PR introduces optimization classes for these situations. No public >> APIs are changed. > > Tingjun Yuan has updated the pull request incrementally with one additional > commit since the last revision: > > Set.copyOf: need defensive copy I'm wary of the impact of adding new wrapper classes. It may make the factory methods slightly slower due additional checks, but also risks increasing the number of classes at various call-sites which might upset call site inlining. An alternative design which would avoid adding more classes could be to add package-private accessors to the existing `Unmodifiable/Synchronized` wrapper classes so that `EnumSet/-Map` can retrieve the underlying set of an unmodifiable or synchronized `Set` or `Map` and then use it directly for these bulk operations. Then you'd contain the additional overhead to `EnumSet/-Map`. - PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478035549
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Mar 2023 15:23:53 GMT, Claes Redestad wrote: > An alternative design which would avoid adding more classes could be to add > package-private accessors to the existing `Unmodifiable/Synchronized` wrapper > classes so that `EnumSet/-Map` can retrieve the underlying set of an > unmodifiable or synchronized `Set` or `Map` and then use it directly for > these bulk operations. Then you'd contain the additional overhead to > `EnumSet/-Map`. Another alternative, in cases where `arg.getClass() == X.class` can be safely substituted with `requireNonNull(arg) instanceof X`, is a marker interface. - PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478253385
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote:
>> Currently, the two subclasses of `java.util.EnumSet` optimize bulk
>> operations when the argument is also a `EnumSet`, but there is no such
>> optimization for wrapper sets (returned by `Collections.unmodifiableSet`,
>> `Collections.synchronizedSet`, etc.) and immutable sets (returned by
>> `Set.of` methods) of `Enum`s.
>>
>> This PR introduces optimization classes for these situations. No public
>> APIs are changed.
>
> Tingjun Yuan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Set.copyOf: need defensive copy
Thanks again for reviewing. Correct me if I'm wrong.
@cl4es I need to add these new wrapper classes
`{Unmodifiable,Synchronized}{Regular,Jumbo}EnumSet` to implement interfaces
`{Regular,Jumbo}EnumSetCompatible`, just like `*RandomAccessList` which appear
to implement `RandomAccess`. I don't think removing these wrapper classes and
introducing accessors is a good idea, because I will have to check for the
wrapper classes in bulk operation methods, which are used more frequently.
@pavelrappo `{Unmodifiable,Sychronized}Set` also have subclasses implementing
`SortedSet` and `NavigableSet`, I'm not sure whether `instanceof` is safe for
these subclasses.
-
PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478658361
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote:
>> Currently, the two subclasses of `java.util.EnumSet` optimize bulk
>> operations when the argument is also a `EnumSet`, but there is no such
>> optimization for wrapper sets (returned by `Collections.unmodifiableSet`,
>> `Collections.synchronizedSet`, etc.) and immutable sets (returned by
>> `Set.of` methods) of `Enum`s.
>>
>> This PR introduces optimization classes for these situations. No public
>> APIs are changed.
>
> Tingjun Yuan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Set.copyOf: need defensive copy
The `RandomAccess` `SortedSet` `NaviagableSet` implementations are public to
users while `RegularEnumSetCompatible` and `JumboEnumSetCompatible` aren't. So
I guess we can implement another interface for these wrappers to retrieve their
backing instances, like:
interface WrapperCollection {
Collection getBacking();
}
and thus:
interface RegularEnumSetCompatible {
static RegularEnumSetCompatible tryConvert(Collection coll) {
if (coll instanceof RegularEnumSetCompatible compat) return compat;
if (coll instanceof WrapperCollection wrap) return
tryConvert(wrap.getBacking());
return null;
}
}
Adding extra `getClass() == UnmodifiableRegularEnumSet.class` indeed will
affect most users, which rarely are enum set compatibles.
-
PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478689026
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote:
>> Currently, the two subclasses of `java.util.EnumSet` optimize bulk
>> operations when the argument is also a `EnumSet`, but there is no such
>> optimization for wrapper sets (returned by `Collections.unmodifiableSet`,
>> `Collections.synchronizedSet`, etc.) and immutable sets (returned by
>> `Set.of` methods) of `Enum`s.
>>
>> This PR introduces optimization classes for these situations. No public
>> APIs are changed.
>
> Tingjun Yuan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Set.copyOf: need defensive copy
Also, in JEP 441, there is pattern matching support for different enums
implementing one interface in one switch, see the enum constants section there.
I wonder how useful this is, and how often do we have interfaces with more than
one enum implementations (e.g. StandardOptions/ExtendedOptions)
src/java.base/share/classes/java/util/ImmutableCollections.java line 1110:
> 1108: static final JavaLangAccess JLA =
> SharedSecrets.getJavaLangAccess();
> 1109:
> 1110: @Stable
Useless
src/java.base/share/classes/java/util/ImmutableCollections.java line 1174:
> 1172: static final class ImmutableRegularEnumSet>
> extends AbstractImmutableEnumSet
> 1173: implements RegularEnumSetCompatible, Serializable {
> 1174: @Stable
Useless
src/java.base/share/classes/java/util/ImmutableCollections.java line 1267:
> 1265: final long[] elements;
> 1266:
> 1267: @Stable
Useless
-
PR Review: https://git.openjdk.org/jdk/pull/12498#pullrequestreview-1351490416
PR Review Comment: https://git.openjdk.org/jdk/pull/12498#discussion_r1144062917
PR Review Comment: https://git.openjdk.org/jdk/pull/12498#discussion_r1144064197
PR Review Comment: https://git.openjdk.org/jdk/pull/12498#discussion_r1144062709
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote: >> Currently, the two subclasses of `java.util.EnumSet` optimize bulk >> operations when the argument is also a `EnumSet`, but there is no such >> optimization for wrapper sets (returned by `Collections.unmodifiableSet`, >> `Collections.synchronizedSet`, etc.) and immutable sets (returned by >> `Set.of` methods) of `Enum`s. >> >> This PR introduces optimization classes for these situations. No public >> APIs are changed. > > Tingjun Yuan has updated the pull request incrementally with one additional > commit since the last revision: > > Set.copyOf: need defensive copy If this level of complexity is indeed needed to get whatever improvement you're after then I don't see how this can be worth its weight. Microbenchmarking might help support your case here, but assessing the potential performance costs from gradually increasing the number of classes floating around at various call sites in arbitrary applications is hard. Thus it is something we need to be very careful not to do without solid evidence. - PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478731354
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Feb 2023 03:39:46 GMT, Tingjun Yuan wrote:
>> Currently, the two subclasses of `java.util.EnumSet` optimize bulk
>> operations when the argument is also a `EnumSet`, but there is no such
>> optimization for wrapper sets (returned by `Collections.unmodifiableSet`,
>> `Collections.synchronizedSet`, etc.) and immutable sets (returned by
>> `Set.of` methods) of `Enum`s.
>>
>> This PR introduces optimization classes for these situations. No public
>> APIs are changed.
>
> Tingjun Yuan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Set.copyOf: need defensive copy
I have deleted `{Regular,Jumbo}EnumSetCompatible` interfaces and introduced
some package-private methods in `AbstractCollection`. This avoids rarely-used
checks from static factories in `Collections` and does not reduce much
performance compared against the previous implementation.
-
PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1478790594
Re: RFR: 8302818: Optimize wrapper sets and immutable sets of Enums [v5]
On Tue, 21 Mar 2023 23:41:44 GMT, Claes Redestad wrote: >> Tingjun Yuan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Set.copyOf: need defensive copy > > If this level of complexity is indeed needed to get whatever improvement > you're after then I don't see how this can be worth its weight. > Microbenchmarking might help support your case here, but assessing the > potential performance costs from gradually increasing the number of classes > floating around at various call sites in arbitrary applications is hard. Thus > it is something we need to be very careful not to do without solid evidence. @cl4es @stuart-marks Thanks for reviewing and commenting. I'm converting this PR to draft until I finish evaluating whether these changes are necessary or not. - PR Comment: https://git.openjdk.org/jdk/pull/12498#issuecomment-1479891838
