8368178: Add specialization of SequencedCollection methods to emptyList, singletonList and nCopies

[amaembo]

Please review this small change. If you have more ideas which classes may miss specializations of SequencedCollection methods, I can add them to this PR as well.

---

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-8368178: Add specialization of SequencedCollection methods to emptyList, singletonList and nCopies (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27406

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back tvaleev! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

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

[mlbridge]

Webrevs

• 01: Full - Incremental (64e7fac2)
• 00: Full (c45761cf)

[pavelrappo]

It's a very sensible change; thanks! The only small, orthogonal quibble inline.

[pavelrappo]

A jtreg comment styled as a javadoc comment is one of my pet peeves. IDE and javac (since JDK 23) treats it as a javadoc comment. For example, such a comment has unknown javadoc tags, or may be considered dangling (which is what happens in your case).

[amaembo]

Thank you for the review. I copied the comment from another test file and was not sure whether an extra asterisk is important for jtreg. You're right, it looks like it can be omitted.

[pavelrappo]

If you have more ideas which classes may miss specializations of SequencedCollection methods, I can add them to this PR as well.

Have you considered something like this?

diff --git a/src/java.base/share/classes/java/util/ImmutableCollections.java b/src/java.base/share/classes/java/util/ImmutableCollections.java
index 38cc45122a2..d8c3499a958 100644
--- a/src/java.base/share/classes/java/util/ImmutableCollections.java
+++ b/src/java.base/share/classes/java/util/ImmutableCollections.java
@@ -352,7 +352,7 @@ public boolean contains(Object o) {
 
         @Override
         public List<E> reversed() {
-            return ReverseOrderListView.of(this, false);
+            return size() < 2 ? this : ReverseOrderListView.of(this, false);
         }
 
         IndexOutOfBoundsException outOfBounds(int index) {
@@ -636,6 +636,11 @@ public int lastIndexOf(Object o) {
             }
         }
 
+        @Override
+        public List<E> reversed() {
+            return size() == 1 ? this : super.reversed();
+        }
+
         @java.io.Serial
         private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
             throw new InvalidObjectException("not serial proxy");

[amaembo]

@pavelrappo re ImmutableCollections, I was thinking about them. One thing that bugs me is that List.copyOf(immutableList) doesn't copy, but List.copyOf(immutableList.reversed()) produces a fresh copy. Probably this is not a very common scenario to optimize, though.

Re List12.reversed() with size == 2: probably it's better to create a fresh list like new List12<>(e1, e0), like

        @SuppressWarnings("unchecked")
        @Override
        public List<E> reversed() {
            return e1 == EMPTY ? this : new List12<>((E) e1, e0);
        }

This way, we have fewer indirections and always return the instance of List12, which may probably help with devirtualization sometimes. The List12 instance size is small. With compressed OOPs it's likely the same size as ReverseOrderListView (two object fields, compared to object+boolean fields in ReverseOrderListView), and the original list could become eligible for GC. One downside of this approach is that listOf2.reversed().reversed() will return a fresh instance, rather than the original one. But I guess it's a rare scenario and the cost is still small. What do you think?

[amaembo]

One more concern is that ReverseOrderListView is not serializable, but List12 and ListN are serializable. With this change, some reversed lists will be serializable while others are not. I don't see anything in the specification regarding the serializability of the reversed lists, but such a discrepancy might be confusing for some users.

[pavelrappo]

@pavelrappo re ImmutableCollections, I was thinking about them. One thing that bugs me is that List.copyOf(immutableList) doesn't copy, but List.copyOf(immutableList.reversed()) produces a fresh copy. Probably this is not a very common scenario to optimize, though.

Re List12.reversed() with size == 2: probably it's better to create a fresh list like new List12<>(e1, e0), like

        @SuppressWarnings("unchecked")
        @Override
        public List<E> reversed() {
            return e1 == EMPTY ? this : new List12<>((E) e1, e0);
        }

This way, we have fewer indirections and always return the instance of List12, which may probably help with devirtualization sometimes. The List12 instance size is small. With compressed OOPs it's likely the same size as ReverseOrderListView (two object fields, compared to object+boolean fields in ReverseOrderListView), and the original list could become eligible for GC. One downside of this approach is that listOf2.reversed().reversed() will return a fresh instance, rather than the original one. But I guess it's a rare scenario and the cost is still small. What do you think?

One more concern is that ReverseOrderListView is not serializable, but List12 and ListN are serializable. With this change, some reversed lists will be serializable while others are not. I don't see anything in the specification regarding the serializability of the reversed lists, but such a discrepancy might be confusing for some users.

---

Your comments are good and thought-through. When dealing with classes like that, it's time to bring a microscope and be concerned with as many aspects as possible.

As for the first comment, List.of and List.copyOf do not specify the class whose instance they return, or even if and how that class overrides reversed(). As such, I think this @implSpec leaves us with no choice but to abide by the roundtrip property (i.e. list.reversed().reversed() == list):

Implementation Requirements:

The implementation in this interface returns a reverse-ordered List view. The reversed() method of the view returns a reference to this List.

As for serializability, it also bugs be. Unmodifiable lists are designed to be consistent and strict. Even though the spec clearly says that it's a bad idea to serialize an unkown list, if a list is serializable some of the times but not others, it might hide bugs:

In cases where the serializability of a collection is not specified, there is no guarantee about the serializability of such collections. In particular, many view collections are not serializable, even if the original collection is serializable.

default List reversed()
Returns a reverse-ordered view of this collection.

I defer this serializability issue to @stuart-marks.

[pavelrappo]

I'll slightly refine my List12 suggestion for your consideration:

    @Override
    public List<E> reversed() {
        return e1 == EMPTY ? this : ReverseOrderListView.of(this, false);
    }

[pavelrappo]

I think this @implSpec leaves us with no choice but to abide by the roundtrip property (i.e. list.reversed().reversed() == list):

On a second thought, no, it is not a specification for List clients. It's a specification for List implementors, so that they can decide if and how they override this method. Clients or implementors don't need to abide by it.

Although, I now wonder why the default implementation is like that. Is the roundtrip property important?

[pavelrappo]

Although, I now wonder why the default implementation is like that.

Perhaps even that is not a mystery. If a view has a reference to the original list anyway, why not use it?

[amaembo]

Yes, implSpec describes the implementation of a concrete default method without requiring anybody to follow the same restrictions. And you're right, returning the original collection is the easiest and most optimal thing one can do. So I think we are not restricted to return this from this.reversed().reversed().

Serialization is already inconsistent. We can't say that the result of reversed() is never serializable, because reversed().reversed() call may again return the serializable collection. In any case, let's wait for @stuart-marks opinion.

[minborg]

In theory, EmptyList and SingletonList could implement SequencedCollection. Same for map.

[amaembo]

@minborg EmptyList already implements it, as any other List:

public interface List<E> extends SequencedCollection<E> { ... }

For EmptyMap (and even SingletonMap and SingletonSet), it's possible, but it will require changing the public interface (Collections::emptyMap will have to return SequencedMap), which may produce binary compatibility issues. Probably we can invent a binary-compatible signature like this:

public static final <K,V,M extends Map<K, V> & SequencedMap<K, V>> M emptyMap() { ... }

But it looks ugly.

[minborg]

@amaembo SequencedMap already implements Map. :-) So, we could say:

public static final <K,V> SequencedMap<K, V> emptyMap() { ... }

An empty map could also "incidentally" implement SequencedMap.

[amaembo]

@minborg as JLS 13.4.15 says,

Changing the result type of a method, or replacing a result type with void, or replacing void with a result type, has the combined effect of deleting the old method and adding a new method with the new result type or newly void result

Changing the result type from Map to SequencedMap will modify the binary signature, so compiled classes that used this method previously will fail with NoSuchMethodError. That's why simply changing Map to SequencedMap is not quite possible. My trick with M extends Map<K, V> & SequencedMap<K, V> employs the fact that the type parameter gets erased to its first bound, which is Map, so such kind of return type replacement is safe (but ugly).

[pavelrappo]

@minborg as JLS 13.4.15 says,

Changing the result type of a method, or replacing a result type with void, or replacing void with a result type, has the combined effect of deleting the old method and adding a new method with the new result type or newly void result

Changing the result type from Map to SequencedMap will modify the binary signature, so compiled classes that used this method previously will fail with NoSuchMethodError. That's why simply changing Map to SequencedMap is not quite possible. My trick with M extends Map<K, V> & SequencedMap<K, V> employs the fact that the type parameter gets erased to its first bound, which is Map, so such kind of return type replacement is safe (but ugly).

Thanks for that JLS reference, Tagir.

@minborg, one would think they can make a method accept wider arguments or return narrower results. After all, it matches basic subtype intuitions. Yet, one cannot do that; they will get java.lang.NoSuchMethodError from code that uses the old method. FWIW, I tripped on it myself a few times.

[minborg]

@minborg as JLS 13.4.15 says,

Changing the result type of a method, or replacing a result type with void, or replacing void with a result type, has the combined effect of deleting the old method and adding a new method with the new result type or newly void result

Changing the result type from Map to SequencedMap will modify the binary signature, so compiled classes that used this method previously will fail with NoSuchMethodError. That's why simply changing Map to SequencedMap is not quite possible. My trick with M extends Map<K, V> & SequencedMap<K, V> employs the fact that the type parameter gets erased to its first bound, which is Map, so such kind of return type replacement is safe (but ugly).

Thanks for that JLS reference, Tagir.

@minborg, one would think they can make a method accept wider arguments or return narrower results. After all, it matches basic subtype intuitions. Yet, one cannot do that; they will get java.lang.NoSuchMethodError from code that uses the old method. FWIW, I tripped on it myself a few times.

Yeah, I get that. Another alternative would be not to expose the type in the API, but instead let the map instance returned from the method implement SequencedMap. A dynamic instanceof would then potentially benefit from this. Maybe there are situations that are less good as well?

[RogerRiggs]

Java indentation is 4 spaces. tnx.

[stuart-marks]

@amaembo @pavelrappo

Hm, a lot of issues to try to address. :-)

First, the initial proposal of adding overrides to EmptyList, SingletonList, and CopiesList is reasonable. No complications there.

On the serializablity of reversed views: in general, the view collections aren't serializable. This also includes things like subList, entrySet, and so forth. The design intent is mostly that they be used temporarily for iteration or copying, and typically aren't stored as the state of some other object. Now that doesn't prohibit a reversed view from being serializable. As Tagir noted, if a list is serializable, and list.reversed().reversed() returns the original list, the return value of reversed() will indeed be serializable. Notable exceptions are the views of TreeMap, which are serializable. Not sure why that was the case.

While the spec of the reversed() method is silent on serializability, List.copyOf()'s spec includes a bunch of requirements including serializability, so it pretty much needs to make a copy reversed views that aren't serializable.

Finally, I think you straightened it out, but as you noted, the @implSpec for List.reversed() specifies that calling reversed() on the reversed view returns the original list. This applies only to the implementation of the reversed() method on the view returned by the default method, but it's not a requirement inherited by all implementers of the reversed() method. Most implementations indeed do this, but some don't -- like the aforementioned TreeMap.