On Thu, 9 Feb 2023 16:20:31 GMT, Tingjun Yuan <d...@openjdk.org> 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. Just curious, how does this enum set change affect the performance of construction of regular sets via Set.of calls? On the JBS there's https://bugs.openjdk.org/browse/JDK-8145048 which appears to fit your changes. Just took a peek again. Besides the critical error of not throwing IAE for duplicate inputs (which you should probably add new test cases for), I think the 2 implementations share the same `toArray` `hashCode`, and the `contains` logic can be pulled to call an abstract `contains(int ordinal)` instead. Have you submitted an issue at https://bugreport.java.com/bugreport/ ? It will show up a few days later on the JBS. If not, I can create an issue on the JBS for you too as long as you provide a prompt. Created as https://bugs.openjdk.org/browse/JDK-8302818 I strongly recommend you to add tests of pure-enum and mixed-enum immutable set creation to ensure your code is logically correct. You seems to be developing the JDK in some weird way as you upload changes directly to GitHub Web UI. I recommend developing according to the docs: see [IDE](https://github.com/openjdk/jdk/blob/master/doc/ide.md) and [building](https://github.com/openjdk/jdk/blob/master/doc/building.md) docs. Also, try set your console's code page to 437 (US-ASCII), as some non-English consoles have caused build failures for me before. If you need help, you can leave this PR as a work in progress (so we don't spam the openjdk mailing lists) and I can answer your questions about the setup. src/java.base/share/classes/java/util/Collections.java line 1153: > 1151: return (Set<T>) s; > 1152: } > 1153: if (s instanceof RegularEnumSetCompatible) { Why not use `if (s instanceof RegularEnumSetCompatible es)` instead? src/java.base/share/classes/java/util/Collections.java line 1194: > 1192: private static final long serialVersionUID = > -1110577510253015312L; > 1193: > 1194: final RegularEnumSetCompatible<? extends E> es; Can't you just use `<E>` as the type argument instead, for all occurrences? Can avoid a cast in `elementType()` too, and it's compatible argument to superclass constructor. src/java.base/share/classes/java/util/Collections.java line 2409: > 2407: public long[] elements() { > 2408: synchronized (mutex) { > 2409: return es.elements(); Is this really safe? `elements()` in `JumboEnumSet` returns the backing array, so this returned array is mutable. Should perform `es.elements().clone()` for more safety. src/java.base/share/classes/java/util/ImmutableCollections.java line 904: > 902: @SuppressWarnings({"varargs", "unchecked", "rawtypes"}) > 903: static <E> Set<E> setFromArray(E... input) { > 904: if (input.length == 0) { This special case can be removed, as no caller calls with an array with length <= 2 src/java.base/share/classes/java/util/ImmutableCollections.java line 913: > 911: try { > 912: // Sorry, I have to use a raw type here. > 913: es = EnumSet.copyOf((Collection)Arrays.asList(input)); // > rejects null This fails to throw `IllegalArgumentException` on duplicated elements. src/java.base/share/classes/java/util/ImmutableCollections.java line 1131: > 1129: long lastReturned = 0; > 1130: final Enum<?>[] universe > 1131: = > SharedSecrets.getJavaLangAccess().getEnumConstantsShared(elementType); I think the convention is to put `SharedSecrets.getJavaLangAccess()` instance in a static final field like `JLA` than calling it every time src/java.base/share/classes/java/util/ImmutableCollections.java line 1178: > 1176: return (es.elements() & ~elements) == 0; > 1177: } else { > 1178: for (Object o : c) { Can just call super here src/java.base/share/classes/java/util/JumboEnumSetCompatible.java line 38: > 36: ImmutableCollections.ImmutableJumboEnumSet { > 37: Class<E> elementType(); > 38: long[] elements(); Since long arrays are mutable (unlike the `long elements()` in regular set), might add a note about this, as enum sets never throws CMEs ------------- PR: https://git.openjdk.org/jdk/pull/12498
