Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

6323374: (coll) Optimize Collections.unmodifiable* and synchronized* #2596

Closed

Conversation

igraves
Copy link
Member

@igraves igraves commented Feb 16, 2021

Modify the unmodifiable* methods in java.util.Collections to be idempotent. That is, when given an immutable collection from java.util.ImmutableCollections or java.util.Collections, these methods will return the reference instead of creating a new immutable collection that wraps the existing one.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-6323374: (coll) Optimize Collections.unmodifiable* and synchronized*

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596
$ git checkout pull/2596

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 16, 2021

👋 Welcome back igraves! 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.

@igraves
Copy link
Member Author

@igraves igraves commented Feb 16, 2021

/csr

@openjdk openjdk bot added rfr csr labels Feb 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 16, 2021

@igraves this pull request will not be integrated until the CSR request JDK-8261677 for issue JDK-6323374 has been approved.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 16, 2021

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

@openjdk openjdk bot added the core-libs label Feb 16, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 16, 2021

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Please add a space after if -> if .

src/java.base/share/classes/java/util/Collections.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/util/Collections.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/util/Collections.java Outdated Show resolved Hide resolved
Copy link
Contributor

@liach liach left a comment

In addition year for license of Collections.java needs an update too

cl4es
cl4es approved these changes Feb 22, 2021
Copy link
Member

@cl4es cl4es left a comment

This looks good to me.

@jddarcy
Copy link
Member

@jddarcy jddarcy commented Feb 23, 2021

Is there any behavior change here that merits a CSR review?

@cl4es
Copy link
Member

@cl4es cl4es commented Feb 23, 2021

Is there any behavior change here that merits a CSR review?

Maybe. The one observable change is that calling Collections.bar(foo) with a foo that is already a bar will return the instance rather than unnecessarily wrap it. This could change semantics in applications inadvertently or deliberately relying on identity.

@igraves
Copy link
Member Author

@igraves igraves commented Feb 23, 2021

Is there any behavior change here that merits a CSR review?

Maybe. The one observable change is that calling Collections.bar(foo) with a foo that is already a bar will return the instance rather than unnecessarily wrap it. This could change semantics in applications inadvertently or deliberately relying on identity.

Yes. The CSR was to consider primarily this case. Probably out of an abundance of caution here. @stuart-marks may have another case to consider.

@stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Feb 23, 2021

Is there any behavior change here that merits a CSR review?

Yes. See my comments in the bug report:

https://bugs.openjdk.java.net/browse/JDK-6323374?focusedCommentId=14296330&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14296330

There is not only the issue of the identity of the object returned, but the change is also observable in the serialized form. Most people would consider the change (less nesting) to be an improvement, but the change is observable, and as we know any observable behavior can become depended upon by applications.

@stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Feb 24, 2021

Code changes all look good. I'm thinking that we should add @implNote clauses to all the docs of the affected methods, saying something like "This method may return its argument if it is already unmodifiable." Usually it's reasonable to leave these kinds of behaviors unspecified (and we do so elsewhere) but since this is a change in long-standing behavior, it seems reasonable to highlight it explicitly. I don't think we want to specify it though, because of the issue with ImmutableCollections (as discussed previously) and possible future tuning of behavior regarding the various Set and Map subinterfaces. (For example, C.unmodifiableSet(arg) could return arg if it's an UnmodifiableNavigableSet.)

The test seems to have a lot of uncomfortable dependencies, both explicit and implicit, on the various ImmutableCollection and UnmodifiableX implementation classes. Would it be sufficient to test various instances for reference equality and inequality instead? For example, something like

var list0 = List.of();
var list1 = Collections.unmodifiableList(list0);
var list2 = Collections.unmodifiableList(list1);
assertNotSame(list0, list1);
assertSame(list1, list2);

This would avoid having to write test cases that cover various internal classes. The ImmutableCollections classes have been reorganized in the past, and while we don't have any plans to do so again at the moment, there is always the possibility of it happening again.

One could write out all the different cases "by hand" but there are rather a lot of them. It might be fruitful to extract the "wrap once, wrap again, assertNotSame, assertSame" logic into a generic test and drive it somehow with a data provider that provides the base instance and a wrapper function.

@igraves
Copy link
Member Author

@igraves igraves commented Feb 26, 2021

Per @stuart-marks I rewrote the tests using some of his suggestions, which substantially reduced dependencies and test size.

Copy link
Member

@stuart-marks stuart-marks left a comment

The @implNote additions are good, and the test rewrite looks good too.

* @param <T> the class of the objects in the set
* @param s the sorted set for which an unmodifiable view is to be
* returned.
* @return an unmodifiable view of the specified sorted set.
*/
public static <T> SortedSet<T> unmodifiableSortedSet(SortedSet<T> s) {
if (s.getClass() == UnmodifiableSortedSet.class) {
Copy link
Member

@jddarcy jddarcy Mar 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a check like this also included "|| == UnmodifiableNavigableSet.class" or was there an explicit decision that the cost/benefit is not worthwhile, unlike in the case of unmodifiableList below?

Copy link
Member Author

@igraves igraves Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point. The case of unmodifiableList is such because the method can return two different classes depending the nature of the argument. I feel as though if we made this change here, we should consider doing the same check for vanilla unmodifiableSet to ensure it, too, doesn't wrap its subclasses. I'm amenable to this.

Copy link
Member Author

@igraves igraves Mar 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To the second part of the question, there was no explicit cost/benefit analysis RE List or this case.

@openjdk openjdk bot removed the csr label Mar 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 3, 2021

@igraves This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

6323374: (coll) Optimize Collections.unmodifiable* and synchronized*

Reviewed-by: redestad, smarks, darcy

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 331 new commits pushed to the master branch:

  • ee09bad: 8262300: jpackage app-launcher fails on linux when using JDK11 based runtime
  • 351889f: 8262508: Vector API's ergonomics is incorrect
  • 718d4d4: 8262989: Vectorize VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods
  • c8b23e2: 8262064: Make compiler/ciReplay tests ignore lambdas in compilation replay
  • 02fbcb5: 8261532: Archived superinterface class cannot be accessed
  • 109af7b: 8261518: jpackage looks for main module in current dir when there is no module-path
  • e61a3ba: 8239386: handle ContendedPaddingWidth in vm_version_aarch64
  • f56c918: 8262837: handle split_USE correctly
  • bd1a806: 8263040: fix for JDK-8262122 fails validate-source
  • a6427c8: 8259709: Disable SHA-1 XML Signatures
  • ... and 321 more: https://git.openjdk.java.net/jdk/compare/48c932e1f1e5a79a28211f72dc9f10d8fd30b955...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs, @cl4es, @stuart-marks, @jddarcy) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Mar 3, 2021
@stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Mar 4, 2021

Hm. I had thought of this previously but I was a bit suspicious, and it didn't seem like it would make much difference, so I didn't say anything. But thinking about this further, the following issues arose:

  1. Suppose you have an UnmodifiableSortedSet and call unmodifiableSet() on it, it returns its argument, and then you hand it out. Its static type is Set but it's really a SortedSet, which means somebody can downcast it and get its comparator. This leaks some information. Offhand this doesn't look dangerous, but it's a bit of a surprise.

  2. Thinking about this further, this allows heap pollution. (Ian, turns out our conversation from the other day wasn't just an idle digression.) If we have class X and class Y extends X, then SortedSet<Y> cannot safely be cast to an unmodifiable SortedSet<X>. That's because comparator() will return Comparator<? super X> which is incorrect, since the actual comparator might be of type Comparator<Y>. Actually the headSet(), subSet(), and tailSet() methods also cause problems, because they both consume and produce the collection element type E.

  3. This can actually happen in practice with code in the JDK! PriorityBlockingQueue's copy constructor does exactly the above. It takes a Collection and does an instanceof check to see if it's a SortedSet; if it is, it does a downcast and uses its comparator. Thus if we do the following:

    SortedSet<Integer> set1 = new TreeSet<>(Integer::compare);
    Set<Number> set2 = Collections.unmodifiableSet(set1); // hypothetical version that returns its argument
    PriorityBlockingQueue<Number> pbq = new PriorityBlockingQueue<>(set2);
    pbq.addAll(Arrays.asList(1.0, 2.0));

this compiles without warnings, but it results in ClassCastException. The culprit is the new upcast that potentially allows SortedSet<? extends T> to be cast to Set<T>, which is slipped in under the already-existing warnings suppression.

In any case, the extra checking in the unmodifiableSortedSet and -Map methods needs to be taken out. Offhand I don't know if there's a similar issue between unmodifiableSortedSet and a NavigableSet (resp., Map), but on general principle I'd say to take it out too. It's likely not buying much anyway.

The UnmodifiableList and UnmodifiableRandomAccessList stuff should stay, since that's how the RandomAccess marker interface is preserved.

@igraves
Copy link
Member Author

@igraves igraves commented Mar 4, 2021

Good thought on the heap pollution. I had only been considering point 1. I was thinking that perhaps if we could catch some of the wrapping of subclasses we might be able to guard against situations where rewrapping could occur if we were interleaving calls between subclass and superclass wrapper methods. That seems like a bit of a reach and in light of your point about heap pollution I think it makes sense to walk back the changes and stay with the original.

Likewise agree on the point about Lists.

@jddarcy
Copy link
Member

@jddarcy jddarcy commented Mar 4, 2021

If the checks for Navigable set and the like are omitted, I'd prefer to a comment in the sources noting this is intention as a Navigable set is-a Sorted set.

@igraves
Copy link
Member Author

@igraves igraves commented Mar 4, 2021

Added comments to the relevant methods noting our intention not to check for subclasses with a little note why.

jddarcy
jddarcy approved these changes Mar 4, 2021
@igraves
Copy link
Member Author

@igraves igraves commented Mar 5, 2021

/integrate

@openjdk openjdk bot added the sponsor label Mar 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 5, 2021

@igraves
Your change (at version 47113ba) is now ready to be sponsored by a Committer.

@stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Mar 5, 2021

/sponsor

@openjdk openjdk bot closed this Mar 5, 2021
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Mar 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 5, 2021

@stuart-marks @igraves Since your change was applied there have been 331 commits pushed to the master branch:

  • ee09bad: 8262300: jpackage app-launcher fails on linux when using JDK11 based runtime
  • 351889f: 8262508: Vector API's ergonomics is incorrect
  • 718d4d4: 8262989: Vectorize VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods
  • c8b23e2: 8262064: Make compiler/ciReplay tests ignore lambdas in compilation replay
  • 02fbcb5: 8261532: Archived superinterface class cannot be accessed
  • 109af7b: 8261518: jpackage looks for main module in current dir when there is no module-path
  • e61a3ba: 8239386: handle ContendedPaddingWidth in vm_version_aarch64
  • f56c918: 8262837: handle split_USE correctly
  • bd1a806: 8263040: fix for JDK-8262122 fails validate-source
  • a6427c8: 8259709: Disable SHA-1 XML Signatures
  • ... and 321 more: https://git.openjdk.java.net/jdk/compare/48c932e1f1e5a79a28211f72dc9f10d8fd30b955...master

Your commit was automatically rebased without conflicts.

Pushed as commit dbef0ec.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
8 participants