8302818: Optimize wrapper sets and immutable sets of Enums #12498

yuantj · 2023-02-09T16:20:31Z

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 Enums.

This PR introduces optimization classes for these situations. No public APIs are changed.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8302818: Optimize wrapper sets and immutable sets of Enums (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/12498/head:pull/12498
$ git checkout pull/12498

Update a local copy of the PR:
$ git checkout pull/12498
$ git pull https://git.openjdk.org/jdk.git pull/12498/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12498

View PR using the GUI difftool:
$ git pr show -t 12498

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12498.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-02-09T16:22:09Z

👋 Welcome back yuantj! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-02-09T16:24:52Z

@yuantj The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

liach · 2023-02-09T19:37:03Z

Just curious, how does this enum set change affect the performance of construction of regular sets via Set.of calls?

liach · 2023-02-09T19:41:24Z

On the JBS there's https://bugs.openjdk.org/browse/JDK-8145048 which appears to fit your changes.

yuantj · 2023-02-09T19:57:19Z

Just curious, how does this enum set change affect the performance of construction of regular sets via Set.of calls?

See ImmutableCollections.setFromArray, it relies on the fact that EnumSet.copyOf will throw a ClassCastException if the elements are not enum constants of a same enum class. Trying to catch an exception is a bit expensive (so maybe I can optimize that).

yuantj · 2023-02-09T20:01:09Z

On the JBS there's https://bugs.openjdk.org/browse/JDK-8145048 which appears to fit your changes.

I only enhanced EnumSet, but not EnumMap (yet) so I don't think it fits my changes.

src/java.base/share/classes/java/util/ImmutableCollections.java

liach · 2023-02-19T04:31:33Z

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.

src/java.base/share/classes/java/util/Collections.java

src/java.base/share/classes/java/util/ImmutableCollections.java

src/java.base/share/classes/java/util/JumboEnumSetCompatible.java

yuantj · 2023-02-19T06:01:43Z

@liach Thanks for reviewing. I've fixed the issues you've mentioned.

liach · 2023-02-19T06:38:13Z

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.

yuantj · 2023-02-19T06:40:03Z

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.

No I haven't. Submit one for me please. Thanks!

mlbridge · 2023-02-20T12:15:18Z

Webrevs

src/java.base/share/classes/java/util/Set.java

liach · 2023-02-20T13:16:09Z

src/java.base/share/classes/java/util/EnumSet.java

+                result.addAll(c);   // optimized for compatible sets
+            } else {
+                while (i.hasNext())
+                    result.add(i.next());


Can't we just use addAll in all cases?

I wonder why the original code didn't use addAll, so I didn't change it. 😂

I wonder why the original code didn't use addAll, so I didn't change it. 😂

It seems to be okay to use addAll in all cases. Do we need extra test cases for this?

It seems to be okay to use `addAll` in all cases.

bridgekeeper · 2023-03-21T09:18:54Z

@yuantj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

cl4es · 2023-03-21T15:23:53Z

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.

pavelrappo · 2023-03-21T16:56:07Z

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.

yuantj · 2023-03-21T22:12:37Z

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.

liach · 2023-03-21T22:40:31Z

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.

liach

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)

liach · 2023-03-21T22:46:58Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+        @Stable
+        final long[] elements;
+
+        @Stable


liach · 2023-03-21T22:47:20Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+            implements Serializable permits ImmutableRegularEnumSet, ImmutableJumboEnumSet {
+        static final JavaLangAccess JLA = SharedSecrets.getJavaLangAccess();
+
+        @Stable


liach · 2023-03-21T22:49:40Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+    @jdk.internal.ValueBased
+    static final class ImmutableRegularEnumSet<E extends Enum<E>> extends AbstractImmutableEnumSet<E>
+            implements RegularEnumSetCompatible<E>, Serializable {
+        @Stable


cl4es · 2023-03-21T23:41:44Z

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.

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.

yuantj · 2023-03-22T01:17:35Z

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.

mlbridge · 2023-03-22T08:09:25Z

Mailing list message from - on core-libs-dev:

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 <duke at openjdk.org> wrote:

ExE-Boss

The Immutable*ESIterator.lastReturned* fields can be made into locals of Immutable*ESIterator::next(), as the only reason those fields exist in RegularEnumSet.EnumSetIterator and JumboEnumSet.EnumSetIterator is to support Iterator::remove(), which is not a valid operation on immutable enum sets:

ExE-Boss · 2023-03-22T14:23:30Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+
+        private final class ImmutableRESIterator implements Iterator<E> {
+            long unseen = elements;
+            long lastReturned = 0;


Suggested change

long lastReturned = 0;

ExE-Boss · 2023-03-22T14:23:45Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+            public E next() {
+                if (unseen == 0)
+                    throw new NoSuchElementException();
+                lastReturned = unseen & -unseen;


Suggested change

lastReturned = unseen & -unseen;

long lastReturned = unseen & -unseen;

Suggested change

lastReturned = unseen & -unseen;

long lastReturned = Long.lowestOneBit(unseen);

More descriptive. Same for below.

ExE-Boss · 2023-03-22T14:29:19Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+            long lastReturned = 0;
+            int lastReturnedIndex = 0;


Suggested change

long lastReturned = 0;

int lastReturnedIndex = 0;

ExE-Boss · 2023-03-22T14:30:58Z

src/java.base/share/classes/java/util/ImmutableCollections.java

+                lastReturned = unseen & -unseen;
+                lastReturnedIndex = unseenIndex;


Suggested change

lastReturned = unseen & -unseen;

lastReturnedIndex = unseenIndex;

long lastReturned = unseen & -unseen;

int lastReturnedIndex = unseenIndex;

stuart-marks · 2023-03-22T16:17:46Z

I agree with Claes here in my skepticism of the usefulness of this sort of change. It seems like it's adding lots of complexity. In addition, it also seems like there's a lot of churn in the design. I'd suggest returning this PR to draft status until it can be determined that there's a clear, net improvement.

yuantj · 2023-03-22T16:28:14Z

@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.

bridgekeeper · 2023-05-17T21:36:36Z

@yuantj This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2023-07-12T22:34:39Z

@yuantj This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

yuantj added 7 commits February 9, 2023 23:49

Create RegularEnumSetCompatible.java

e6336b1

Create JumboEnumSetCompatible.java

452bcea

Update Collections.java

92e3375

Update ImmutableCollections.java

204b2e0

Update Set.java

13c3032

Update JumboEnumSet.java

d5ed0e5

Update RegularEnumSet.java

f9cc2ae

openjdk bot added the core-libs label Feb 9, 2023

Fix whitespace errors

940de6e

yuantj marked this pull request as ready for review February 10, 2023 10:27

yuantj marked this pull request as draft February 11, 2023 12:58

liach reviewed Feb 19, 2023

View reviewed changes

src/java.base/share/classes/java/util/ImmutableCollections.java Outdated Show resolved Hide resolved

liach reviewed Feb 19, 2023

View reviewed changes

src/java.base/share/classes/java/util/ImmutableCollections.java Outdated Show resolved Hide resolved

liach reviewed Feb 19, 2023

View reviewed changes

src/java.base/share/classes/java/util/ImmutableCollections.java Outdated Show resolved Hide resolved

liach reviewed Feb 19, 2023

View reviewed changes

yuantj added 3 commits February 19, 2023 13:58

Update ImmutableCollections.java

7a28127

Update Collections.java

cb5b439

Update JumboEnumSetCompatible.java

b61a59f

add permits clause in AbstractImmutableEnumSet

7987e45

fix error

3fe7b96

add import in SetFactories.java

7f704c3

yuantj marked this pull request as ready for review February 20, 2023 12:09

openjdk bot added the rfr label Feb 20, 2023

Optimize EnumSet.copyOf and Set.copyOf

41f4a90

liach reviewed Feb 20, 2023

View reviewed changes

yuantj added 3 commits February 20, 2023 21:38

Use addAll in EnumSet.copyOf

098b3ff

It seems to be okay to use `addAll` in all cases.

Fix Set.copyOf

b9699bf

Set.copyOf: need defensive copy

0e0b2bf

liach reviewed Mar 21, 2023

View reviewed changes

openjdk bot removed the rfr label Mar 22, 2023

Fix a whitespace error

4a7299b

openjdk bot added the rfr label Mar 22, 2023

ExE-Boss reviewed Mar 22, 2023

View reviewed changes

yuantj marked this pull request as draft March 22, 2023 16:29

openjdk bot removed the rfr label Mar 22, 2023

bridgekeeper bot closed this Jul 12, 2023

	lastReturned = unseen & -unseen;
	long lastReturned = unseen & -unseen;

	lastReturned = unseen & -unseen;
	long lastReturned = Long.lowestOneBit(unseen);

		lastReturned = unseen & -unseen;
		lastReturnedIndex = unseenIndex;

8302818: Optimize wrapper sets and immutable sets of Enums #12498

8302818: Optimize wrapper sets and immutable sets of Enums #12498

Conversation

yuantj commented Feb 9, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewing

Webrev

bridgekeeper bot commented Feb 9, 2023

openjdk bot commented Feb 9, 2023

liach commented Feb 9, 2023

liach commented Feb 9, 2023

yuantj commented Feb 9, 2023

yuantj commented Feb 9, 2023 • edited Loading

liach commented Feb 19, 2023

yuantj commented Feb 19, 2023

liach commented Feb 19, 2023

yuantj commented Feb 19, 2023

mlbridge bot commented Feb 20, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridgekeeper bot commented Mar 21, 2023

cl4es commented Mar 21, 2023

pavelrappo commented Mar 21, 2023

yuantj commented Mar 21, 2023

liach commented Mar 21, 2023 • edited Loading

liach left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl4es commented Mar 21, 2023 • edited Loading

yuantj commented Mar 22, 2023

mlbridge bot commented Mar 22, 2023

ExE-Boss left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuart-marks commented Mar 22, 2023

yuantj commented Mar 22, 2023

bridgekeeper bot commented May 17, 2023

bridgekeeper bot commented Jul 12, 2023

yuantj commented Feb 9, 2023 •

edited by openjdk bot

Loading

yuantj commented Feb 9, 2023 •

edited

Loading

mlbridge bot commented Feb 20, 2023 •

edited

Loading

liach commented Mar 21, 2023 •

edited

Loading

liach left a comment •

edited

Loading

cl4es commented Mar 21, 2023 •

edited

Loading

ExE-Boss left a comment •

edited

Loading