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

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Ian Graves-2
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.

-------------

Commit messages:
 - Changing Collections.unmodifiable* to idempotent behavior.

Changes: https://git.openjdk.java.net/jdk/pull/2596/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-6323374
  Stats: 281 lines in 2 files changed: 281 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2596.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

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

Roger Riggs-2
On Tue, 16 Feb 2021 21:57:43 GMT, Ian Graves <[hidden email]> wrote:

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

Please add a space after `if` ->  `if `.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad-2
In reply to this post by Ian Graves-2
On Tue, 16 Feb 2021 21:57:43 GMT, Ian Graves <[hidden email]> wrote:

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

Changes requested by redestad (Reviewer).

src/java.base/share/classes/java/util/Collections.java line 1130:

> 1128:     public static <T> Set<T> unmodifiableSet(Set<? extends T> s) {
> 1129:         if(s.getClass() == UnmodifiableSet.class ||
> 1130:            s.getClass() == ImmutableCollections.Set12.class ||

These might be problematic: `ImmutableCollections.Set*` differs in behavior subtly from `UnmodifiableSet`, e.g., `set.contains(null)` will throw an NPE. I'd limit the check and optimization to `UnmodifiableSet` here

src/java.base/share/classes/java/util/Collections.java line 1315:

> 1313:     public static <T> List<T> unmodifiableList(List<? extends T> list) {
> 1314:         if (list.getClass() == UnmodifiableList.class || list.getClass() == UnmodifiableRandomAccessList.class ||
> 1315:             list.getClass() == ImmutableCollections.List12.class ||

Similar issue here

src/java.base/share/classes/java/util/Collections.java line 1473:

> 1471:     public static <K,V> Map<K,V> unmodifiableMap(Map<? extends K, ? extends V> m) {
> 1472:         if(m.getClass() == UnmodifiableMap.class ||
> 1473:            m.getClass() == ImmutableCollections.Map1.class ||

And here.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

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

Michael Hixson-2
In reply to this post by Ian Graves-2
On Tue, 16 Feb 2021 21:57:43 GMT, Ian Graves <[hidden email]> wrote:

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

src/java.base/share/classes/java/util/Collections.java line 1473:

> 1471:     public static <K,V> Map<K,V> unmodifiableMap(Map<? extends K, ? extends V> m) {
> 1472:         if(m.getClass() == UnmodifiableMap.class ||
> 1473:            m.getClass() == ImmutableCollections.Map1.class ||

(I'm not a reviewer.)

I think this causes a change in behavior to this silly program.

var map1 = Map.of(1, 2);
var map2 = Collections.unmodifiableMap(map1);

try {
  System.out.println("map1 returned " + map1.entrySet().contains(null));
} catch (NullPointerException e) {
  System.out.println("map1 threw");
}

try {
  System.out.println("map2 returned " + map2.entrySet().contains(null));
} catch (NullPointerException e) {
  System.out.println("map2 threw");
}

With JDK 15 the output is:
> map1 threw
> map2 returned false

With this change I think the output will be:
> map1 threw
> map2 threw

It seems unlikely that anyone will be bit by this, but it is a change to behavior and it wasn't called out in the Jira issue, so I felt it was worth mentioning.

I think it is just this one specific case that changes -- only `Map1`, and only `entrySet().contains(null)`.  Other sub-collections like `keySet()` and `values()` and `subList(...)` already throw on `contains(null)` for both the `ImmutableCollections.*` implementation and the `Collections.umodifiable*` wrapper.  `MapN`'s `entrySet().contains(null)` already returns `false` for both.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

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

liach
In reply to this post by Claes Redestad-2
On Tue, 16 Feb 2021 23:18:55 GMT, Claes Redestad <[hidden email]> wrote:

>> 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.
>
> src/java.base/share/classes/java/util/Collections.java line 1130:
>
>> 1128:     public static <T> Set<T> unmodifiableSet(Set<? extends T> s) {
>> 1129:         if(s.getClass() == UnmodifiableSet.class ||
>> 1130:            s.getClass() == ImmutableCollections.Set12.class ||
>
> These might be problematic: `ImmutableCollections.Set*` differs in behavior subtly from `UnmodifiableSet`, e.g., `set.contains(null)` will throw an NPE. I'd limit the check and optimization to `UnmodifiableSet` here

No? This unmodifiable set here just delegates call to the backing field `c`, so all exceptions from `c`'s calls are just delegated, aren't they? The NPE will still be thrown; it's just that the stack trace will be different (i.e. one more level of call in the stacktrace)

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad-2
On Wed, 17 Feb 2021 02:27:57 GMT, liach <[hidden email]> wrote:

>> src/java.base/share/classes/java/util/Collections.java line 1130:
>>
>>> 1128:     public static <T> Set<T> unmodifiableSet(Set<? extends T> s) {
>>> 1129:         if(s.getClass() == UnmodifiableSet.class ||
>>> 1130:            s.getClass() == ImmutableCollections.Set12.class ||
>>
>> These might be problematic: `ImmutableCollections.Set*` differs in behavior subtly from `UnmodifiableSet`, e.g., `set.contains(null)` will throw an NPE. I'd limit the check and optimization to `UnmodifiableSet` here
>
> No? This unmodifiable set here just delegates call to the backing field `c`, so all exceptions from `c`'s calls are just delegated, aren't they? The NPE will still be thrown; it's just that the stack trace will be different (i.e. one more level of call in the stacktrace)

Yes, my bad. There might be more issues similar to the one @michaelhixson points out elsewhere in this PR, so a thorough review is probably needed here.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad-2
In reply to this post by Michael Hixson-2
On Wed, 17 Feb 2021 00:30:09 GMT, Michael Hixson <[hidden email]> wrote:

>> 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.
>
> src/java.base/share/classes/java/util/Collections.java line 1473:
>
>> 1471:     public static <K,V> Map<K,V> unmodifiableMap(Map<? extends K, ? extends V> m) {
>> 1472:         if(m.getClass() == UnmodifiableMap.class ||
>> 1473:            m.getClass() == ImmutableCollections.Map1.class ||
>
> (I'm not a reviewer.)
>
> I think this causes a change in behavior to this silly program.
>
> var map1 = Map.of(1, 2);
> var map2 = Collections.unmodifiableMap(map1);
>
> try {
>   System.out.println("map1 returned " + map1.entrySet().contains(null));
> } catch (NullPointerException e) {
>   System.out.println("map1 threw");
> }
>
> try {
>   System.out.println("map2 returned " + map2.entrySet().contains(null));
> } catch (NullPointerException e) {
>   System.out.println("map2 threw");
> }
>
> With JDK 15 the output is:
>> map1 threw
>> map2 returned false
>
> With this change I think the output will be:
>> map1 threw
>> map2 threw
>
> It seems unlikely that anyone will be bit by this, but it is a change to behavior and it wasn't called out in the Jira issue, so I felt it was worth mentioning.
>
> I think it is just this one specific case that changes -- only `Map1`, and only `entrySet().contains(null)`.  Other sub-collections like `keySet()` and `values()` and `subList(...)` already throw on `contains(null)` for both the `ImmutableCollections.*` implementation and the `Collections.umodifiable*` wrapper.  `MapN`'s `entrySet().contains(null)` already returns `false` for both.

This sounds like an inconsistency between `Map1` and `MapN` that should perhaps be considered a bug that needs fixing. /ping @stuart-marks

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

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

liach
On Wed, 17 Feb 2021 14:21:37 GMT, Claes Redestad <[hidden email]> wrote:

>> src/java.base/share/classes/java/util/Collections.java line 1473:
>>
>>> 1471:     public static <K,V> Map<K,V> unmodifiableMap(Map<? extends K, ? extends V> m) {
>>> 1472:         if(m.getClass() == UnmodifiableMap.class ||
>>> 1473:            m.getClass() == ImmutableCollections.Map1.class ||
>>
>> (I'm not a reviewer.)
>>
>> I think this causes a change in behavior to this silly program.
>>
>> var map1 = Map.of(1, 2);
>> var map2 = Collections.unmodifiableMap(map1);
>>
>> try {
>>   System.out.println("map1 returned " + map1.entrySet().contains(null));
>> } catch (NullPointerException e) {
>>   System.out.println("map1 threw");
>> }
>>
>> try {
>>   System.out.println("map2 returned " + map2.entrySet().contains(null));
>> } catch (NullPointerException e) {
>>   System.out.println("map2 threw");
>> }
>>
>> With JDK 15 the output is:
>>> map1 threw
>>> map2 returned false
>>
>> With this change I think the output will be:
>>> map1 threw
>>> map2 threw
>>
>> It seems unlikely that anyone will be bit by this, but it is a change to behavior and it wasn't called out in the Jira issue, so I felt it was worth mentioning.
>>
>> I think it is just this one specific case that changes -- only `Map1`, and only `entrySet().contains(null)`.  Other sub-collections like `keySet()` and `values()` and `subList(...)` already throw on `contains(null)` for both the `ImmutableCollections.*` implementation and the `Collections.umodifiable*` wrapper.  `MapN`'s `entrySet().contains(null)` already returns `false` for both.
>
> This sounds like an inconsistency between `Map1` and `MapN` that should perhaps be considered a bug that needs fixing. /ping @stuart-marks

2 remarks:
1. MapN's entry set extends abstract set, whose `contains` is null-friendly like https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/AbstractCollection.java#L104
2. The problem of unmodifiable map's entry set not always delegating everything to the backing entry set still exists. https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/Collections.java#L1724 This will bypass the underlying logic when the argument is `null` or not an entry.

The behavior pointed out by michaelhixson is the conglomeration of these 2 unspecified behaviors.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v2]

Ian Graves-2
In reply to this post by Ian Graves-2
> 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.

Ian Graves has updated the pull request incrementally with one additional commit since the last revision:

  If-block formatting

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2596/files
  - new: https://git.openjdk.java.net/jdk/pull/2596/files/e0a06bf7..53e366a9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=00-01

  Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2596.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v2]

Ian Graves-2
In reply to this post by liach
On Wed, 17 Feb 2021 14:37:52 GMT, liach <[hidden email]> wrote:

>> This sounds like an inconsistency between `Map1` and `MapN` that should perhaps be considered a bug that needs fixing. /ping @stuart-marks
>
> 2 remarks:
> 1. MapN's entry set extends abstract set, whose `contains` is null-friendly like https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/AbstractCollection.java#L104
> 2. The problem of unmodifiable map's entry set not always delegating everything to the backing entry set still exists. https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/Collections.java#L1724 This will bypass the underlying logic when the argument is `null` or not an entry.
>
> The behavior pointed out by michaelhixson is the conglomeration of these 2 unspecified behaviors.

This raises some interesting issues and makes me wonder if we should allow a single-wrap of the `ImmutableCollections` classes for now to make this less onerous.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v2]

Ian Graves-2
On Wed, 17 Feb 2021 18:24:39 GMT, Ian Graves <[hidden email]> wrote:

>> 2 remarks:
>> 1. MapN's entry set extends abstract set, whose `contains` is null-friendly like https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/AbstractCollection.java#L104
>> 2. The problem of unmodifiable map's entry set not always delegating everything to the backing entry set still exists. https://github.com/openjdk/jdk/blob/cb84539d56209a6687c4ec71a61fdbe6f06a46ea/src/java.base/share/classes/java/util/Collections.java#L1724 This will bypass the underlying logic when the argument is `null` or not an entry.
>>
>> The behavior pointed out by michaelhixson is the conglomeration of these 2 unspecified behaviors.
>
> This raises some interesting issues and makes me wonder if we should allow a single-wrap of the `ImmutableCollections` classes for now to make this less onerous.

Yes -- I think in response to this it makes more sense to pull the `ImmutableCollections` classes out for now and only focus on the wrapping of the classes within `Collections` so we aren't blocked by studying and rectifying these inconsistencies.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v3]

Ian Graves-2
In reply to this post by Ian Graves-2
> 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.

Ian Graves has updated the pull request incrementally with one additional commit since the last revision:

  Removing ImmutableCollections.* idempotency

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2596/files
  - new: https://git.openjdk.java.net/jdk/pull/2596/files/53e366a9..a9588e9c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=01-02

  Stats: 38 lines in 2 files changed: 7 ins; 17 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2596.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v3]

Ian Graves-2
In reply to this post by Claes Redestad-2
On Wed, 17 Feb 2021 14:14:57 GMT, Claes Redestad <[hidden email]> wrote:

>> No? This unmodifiable set here just delegates call to the backing field `c`, so all exceptions from `c`'s calls are just delegated, aren't they? The NPE will still be thrown; it's just that the stack trace will be different (i.e. one more level of call in the stacktrace)
>
> Yes, my bad. There might be more issues similar to the one @michaelhixson points out elsewhere in this PR, so a thorough review is probably needed here.

Resolving these for now. Going to punt these inconsistencies to another bug before bringing this back in. This change will focus on the idempotency of these methods vs. the classes contained in `Collections`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v3]

jmehrens
In reply to this post by Ian Graves-2
On Wed, 17 Feb 2021 19:12:19 GMT, Ian Graves <[hidden email]> wrote:

>> This raises some interesting issues and makes me wonder if we should allow a single-wrap of the `ImmutableCollections` classes for now to make this less onerous.
>
> Yes -- I think in response to this it makes more sense to pull the `ImmutableCollections` classes out for now and only focus on the wrapping of the classes within `Collections` so we aren't blocked by studying and rectifying these inconsistencies.

Maybe it is not correct for UnmodifiableEntrySet::contains to short circuit?  What if the implementation was changed to:

`public boolean contains(Object o) {
            if (!(o instanceof Map.Entry))
                return c.contains(o); //false, NPE, or CCE
            return c.contains(
                new UnmodifiableEntry<>((Map.Entry<?,?>) o));
}`

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v3]

liach
On Thu, 18 Feb 2021 16:18:42 GMT, jmehrens <[hidden email]> wrote:

>> Yes -- I think in response to this it makes more sense to pull the `ImmutableCollections` classes out for now and only focus on the wrapping of the classes within `Collections` so we aren't blocked by studying and rectifying these inconsistencies.
>
> Maybe it is not correct for UnmodifiableEntrySet::contains to short circuit?  What if the implementation was changed to:
>
> `public boolean contains(Object o) {
>             if (!(o instanceof Map.Entry))
>                 return c.contains(o); //false, NPE, or CCE
>             return c.contains(
>                 new UnmodifiableEntry<>((Map.Entry<?,?>) o));
> }`

This however changes the behavior of unmodifiable maps compared to before; i.e. before for other entry sets, they could not throw exception if the object passed was not map entry; now they can.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v3]

liach
In reply to this post by Ian Graves-2
On Wed, 17 Feb 2021 19:56:01 GMT, Ian Graves <[hidden email]> wrote:

>> 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.
>
> Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
>
>   Removing ImmutableCollections.* idempotency

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]

Ian Graves-2
In reply to this post by Ian Graves-2
> 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.

Ian Graves has updated the pull request incrementally with one additional commit since the last revision:

  Updating Collections.java copyright

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2596/files
  - new: https://git.openjdk.java.net/jdk/pull/2596/files/a9588e9c..ebdd0f85

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2596&range=02-03

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2596.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2596/head:pull/2596

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]

Claes Redestad-2
On Mon, 22 Feb 2021 22:08:56 GMT, Ian Graves <[hidden email]> wrote:

>> 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.
>
> Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
>
>   Updating Collections.java copyright

This looks good to me.

-------------

Marked as reviewed by redestad (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]

Joe Darcy-2
On Mon, 22 Feb 2021 23:39:15 GMT, Claes Redestad <[hidden email]> wrote:

>> Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Updating Collections.java copyright
>
> This looks good to me.

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]

Claes Redestad-2
On Tue, 23 Feb 2021 06:12:54 GMT, Joe Darcy <[hidden email]> wrote:

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/2596
12