RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

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

RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stuart Marks
(also includes 8184690: add Collectors for collecting into unmodifiable List,
Set, and Map)

Hi all,

Here's an updated webrev for this changeset; the previous review thread is here:

     http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html

This webrev includes the following:

* specification revisions to provide clearer definitions of "view" collections,
"unmodifiable" collections, and "unmodifiable views"

* new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods

* new Collectors.toUnmodifiableList, Set, and Map methods

* tests for the new API methods

I've added some assertions that require some independence between the source
collection (or map) and the result of the copyOf() method.

I've made a small but significant change to Set.copyOf compared to the previous
round. Previously, it specified that the first of any equal elements was
preserved. Now, it is explicitly unspecified which of any equals elements is
preserved. This is consistent with Set.addAll, Collectors.toSet, and the newly
added Collectors.toUnmodifiableSet, none of which specify which of duplicate
elements is preserved.

(The outlier here is Stream.distinct, which specifies that the first element of
any duplicates is preserved, if the stream is ordered.)

I've also made some minor wording/editorial changes in response to suggestions
from David Holmes and Roger Riggs. I've kept the wording changes that give
emphasis to "unmodifiable" over "immutable." The term "immutable" is
inextricably intertwined with "persistent" when it comes to data structures, and
I believe we'll be explaining this forever if Java's "immutable" means something
different from everybody else's.

Webrev:

     http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/

Bugs:

     https://bugs.openjdk.java.net/browse/JDK-8177290
         add copy factory methods for unmodifiable List, Set, Map

     https://bugs.openjdk.java.net/browse/JDK-8184690
         add Collectors for collecting into unmodifiable List, Set, and Map

Thanks,

s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Brian Goetz-2
In "View Collections"

Most collections contain elements themselves. By contrast, <i>view
collections</i>
112 * themselves do not contain elements, but instead rely on a backing
collection to
113 * store the actual elements.


This should be: "most collections manage the storage for the elements
they contain.  By contrast, ...".  (All collections contain their
elements -- just ask the `contains()` method!)

Similarly, in:

Correspondingly, any changes made to
126 * the view collection are written through to the backing collection.


you might want to make the connection that views might "handle" the
operation by prohibiting it; view collections need not support changes
if they don't want to.

In

<b>Unmodifiable view collections.</b>

You might want to say explicitly "such as
Collections.unmodifiable{List,Set,...}" when describing unmodifiable
view collections.  THat's of course what you're getting at, but it could
be said more explicitly.

Returns an <a href="#unmodifiable">unmodifiable List</a> containing the
elements
1045 * of the given Collection, in iteration order.

In the _given collection's_ iteration order.

Returns an <a href="#unmodifiable">unmodifiable Map</a> containing the
entries
1664 * of the given Map. the given Map must not be null, and it must not
contain any
1665 * null keys or values. If the given Map is subsequently modified,
the returned
1666 * Map will not reflect such modifications.

Second sentence starts with lower case later.

I might quibble with "subsequently modified", as there is some ambiguity
(what if the map is concurrently modified, subsequent to making the
copyOf() call, but prior to the return from copyOf()?) But I won't ;)

In

296 Collector<T, ?, List<T>> toUnmodifiableList() {
297 return new CollectorImpl<>((Supplier<List<T>>) ArrayList::new,
List::add,
298 (left, right) -> { left.addAll(right); return left; },
299 list -> (List<T>)List.of(list.toArray()),
300 CH_NOID);

why the intermediate array, and not just use copyOf()?  (Is it because
you added copyOf later and didn't go back and update this?)  Same for
other collectors.








On 10/30/2017 6:50 PM, Stuart Marks wrote:

> (also includes 8184690: add Collectors for collecting into
> unmodifiable List, Set, and Map)
>
> Hi all,
>
> Here's an updated webrev for this changeset; the previous review
> thread is here:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>
> This webrev includes the following:
>
> * specification revisions to provide clearer definitions of "view"
> collections, "unmodifiable" collections, and "unmodifiable views"
>
> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory"
> methods
>
> * new Collectors.toUnmodifiableList, Set, and Map methods
>
> * tests for the new API methods
>
> I've added some assertions that require some independence between the
> source collection (or map) and the result of the copyOf() method.
>
> I've made a small but significant change to Set.copyOf compared to the
> previous round. Previously, it specified that the first of any equal
> elements was preserved. Now, it is explicitly unspecified which of any
> equals elements is preserved. This is consistent with Set.addAll,
> Collectors.toSet, and the newly added Collectors.toUnmodifiableSet,
> none of which specify which of duplicate elements is preserved.
>
> (The outlier here is Stream.distinct, which specifies that the first
> element of any duplicates is preserved, if the stream is ordered.)
>
> I've also made some minor wording/editorial changes in response to
> suggestions from David Holmes and Roger Riggs. I've kept the wording
> changes that give emphasis to "unmodifiable" over "immutable." The
> term "immutable" is inextricably intertwined with "persistent" when it
> comes to data structures, and I believe we'll be explaining this
> forever if Java's "immutable" means something different from everybody
> else's.
>
> Webrev:
>
>     http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>
> Bugs:
>
>     https://bugs.openjdk.java.net/browse/JDK-8177290
>         add copy factory methods for unmodifiable List, Set, Map
>
>     https://bugs.openjdk.java.net/browse/JDK-8184690
>         add Collectors for collecting into unmodifiable List, Set, and
> Map
>
> Thanks,
>
> s'marks

Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

roger riggs
In reply to this post by Stuart Marks
Hi Stuart,

Collection.java:

The mix of "should" and "must" in Collection.java can be confusing.
Typically "should" is interpreted as "must" from a conformance point of
view.
For example,

147 * <p>An <i>unmodifiable collection</i> is a collection, all of whose
148 * mutator methods (as defined above) are specified to throw
149 * {@code UnsupportedOperationException}. Such a collection thus
cannot be
150 * modified by calling any methods on it. For a collection to be properly
151 * unmodifiable, any view collections derived from it *must *also be
unmodifiable.
152 * For example, if a List is unmodifiable, the List returned by
153 * {@link List#subList List.subList} *should *also be unmodifiable.

That leaves room for a non-compliant collection which could not
technically be called "unmodifiable".

138 * does not support the operation. Such methods *should *(but are not
required
139 * to) throw an {@code UnsupportedOperationException} if the
invocation would
140 * have no effect on the collection. For example, invoking the
141 * {@link #addAll addAll} method on a collection that does not support
142 * the {@link #add add} operation *should *throw the exception if
143 * the collection passed as an argument is empty.

These two sentences both allow not throwing UOE and requiring UOE. Can
they be worded to be consistent (or similarly qualified.)

Set.java: 706:  Would using "unique elements" clarify the contents of
the result?
Saying "Duplicate elements are permitted," may be misleading.
Perhaps "The given Collection may contain duplicate elements"...

Similarly in Collectors.to toUnmodifiableSet:
"Duplicate elements are allowed," can be misleading, especially the word
allowed.

Collectors.toUnmodifiableMap
      * If the mapped keys
1474      * might have duplicates, use {@link
#toUnmodifiableMap(Function, Function, BinaryOperator)}
1475      * instead.

It would helpful to say why to use the version with the BinaryOperator,
for example to say"
"To determine which of the duplicate elements to retain".  or similar.

Collectors:1606:  impl note:  With three different NPE throws, it is
helpful to use the 2-arg
form of requireNonNull to indicate which parameter offends.

Roger

On 10/30/2017 6:50 PM, Stuart Marks wrote:

> (also includes 8184690: add Collectors for collecting into
> unmodifiable List, Set, and Map)
>
> Hi all,
>
> Here's an updated webrev for this changeset; the previous review
> thread is here:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>
> This webrev includes the following:
>
> * specification revisions to provide clearer definitions of "view"
> collections, "unmodifiable" collections, and "unmodifiable views"
>
> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory"
> methods
>
> * new Collectors.toUnmodifiableList, Set, and Map methods
>
> * tests for the new API methods
>
> I've added some assertions that require some independence between the
> source collection (or map) and the result of the copyOf() method.
>
> I've made a small but significant change to Set.copyOf compared to the
> previous round. Previously, it specified that the first of any equal
> elements was preserved. Now, it is explicitly unspecified which of any
> equals elements is preserved. This is consistent with Set.addAll,
> Collectors.toSet, and the newly added Collectors.toUnmodifiableSet,
> none of which specify which of duplicate elements is preserved.
>
> (The outlier here is Stream.distinct, which specifies that the first
> element of any duplicates is preserved, if the stream is ordered.)
>
> I've also made some minor wording/editorial changes in response to
> suggestions from David Holmes and Roger Riggs. I've kept the wording
> changes that give emphasis to "unmodifiable" over "immutable." The
> term "immutable" is inextricably intertwined with "persistent" when it
> comes to data structures, and I believe we'll be explaining this
> forever if Java's "immutable" means something different from everybody
> else's.
>
> Webrev:
>
>     http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>
> Bugs:
>
>     https://bugs.openjdk.java.net/browse/JDK-8177290
>         add copy factory methods for unmodifiable List, Set, Map
>
>     https://bugs.openjdk.java.net/browse/JDK-8184690
>         add Collectors for collecting into unmodifiable List, Set, and
> Map
>
> Thanks,
>
> s'marks

Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stuart Marks
In reply to this post by Brian Goetz-2
Thanks for the review. Editorial comments accepted. Discussion and responses to some items below.


On 10/31/17 7:25 AM, Brian Goetz wrote:

>
> Returns an <a href="#unmodifiable">unmodifiable Map</a> containing the entries
> 1664 * of the given Map. the given Map must not be null, and it must not
> contain any
> 1665 * null keys or values. If the given Map is subsequently modified, the
> returned
> 1666 * Map will not reflect such modifications.
>
> I might quibble with "subsequently modified", as there is some ambiguity (what
> if the map is concurrently modified, subsequent to making the copyOf() call,
> but prior to the return from copyOf()?)  But I won't ;)
>
Well I actually did think about this, but I couldn't think of anything better to
say. If concurrent modification is permitted, it's conceivable that a
modification made any time during the copy might end up in the returned map. It
depends on the source map's implementation and concurrent modification policy.
So I think the only statement that can be made is about the state of the
returned map after copyOf() returns. That's what "subsequent" means to me, but I
could clarify this if you think it's worth it.

(Similar reasoning for List and Set.)
>
> 296 Collector<T, ?, List<T>> toUnmodifiableList() {
> 297 return new CollectorImpl<>((Supplier<List<T>>) ArrayList::new, List::add,
> 298 (left, right) -> { left.addAll(right); return left; },
> 299 list -> (List<T>)List.of(list.toArray()),
> 300 CH_NOID);
> why the intermediate array, and not just use copyOf()?  (Is it because you
> added copyOf later and didn't go back and update this?)  Same for other
> collectors.
The copyOf method is mostly forced to make a defensive copy, so it's no better.

I think what should happen here eventually is that the elements should be
collected into something like a SpinedBuffer, and then a private interface
should be plumbed into the unmodifiable list implementation that can copy
elements directly from the buffer. This avoids the extra copy. But I'll take
care of that later.

For toUnmodifiableSet and toUnmodifiableMap, I can imagine similar private
interfaces to avoid excess copies, but I think they each still need to build up
full intermediate HashSet/HashMap in order to get rid of duplicates.

s'marks

>
>
>
>
>
>
>
>
> On 10/30/2017 6:50 PM, Stuart Marks wrote:
>> (also includes 8184690: add Collectors for collecting into unmodifiable List,
>> Set, and Map)
>>
>> Hi all,
>>
>> Here's an updated webrev for this changeset; the previous review thread is here:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>
>> This webrev includes the following:
>>
>> * specification revisions to provide clearer definitions of "view"
>> collections, "unmodifiable" collections, and "unmodifiable views"
>>
>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>
>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>
>> * tests for the new API methods
>>
>> I've added some assertions that require some independence between the source
>> collection (or map) and the result of the copyOf() method.
>>
>> I've made a small but significant change to Set.copyOf compared to the
>> previous round. Previously, it specified that the first of any equal elements
>> was preserved. Now, it is explicitly unspecified which of any equals elements
>> is preserved. This is consistent with Set.addAll, Collectors.toSet, and the
>> newly added Collectors.toUnmodifiableSet, none of which specify which of
>> duplicate elements is preserved.
>>
>> (The outlier here is Stream.distinct, which specifies that the first element
>> of any duplicates is preserved, if the stream is ordered.)
>>
>> I've also made some minor wording/editorial changes in response to
>> suggestions from David Holmes and Roger Riggs. I've kept the wording changes
>> that give emphasis to "unmodifiable" over "immutable." The term "immutable"
>> is inextricably intertwined with "persistent" when it comes to data
>> structures, and I believe we'll be explaining this forever if Java's
>> "immutable" means something different from everybody else's.
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>
>> Bugs:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8177290
>>         add copy factory methods for unmodifiable List, Set, Map
>>
>> https://bugs.openjdk.java.net/browse/JDK-8184690
>>         add Collectors for collecting into unmodifiable List, Set, and Map
>>
>> Thanks,
>>
>> s'marks
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stuart Marks
In reply to this post by roger riggs


On 10/31/17 11:24 AM, Roger Riggs wrote:
> Hi Stuart,
>
> Collection.java:
>
> The mix of "should" and "must" in Collection.java can be confusing.
> Typically "should" is interpreted as "must" from a conformance point of view.

"Must" is a requirement, and "should" is a recommendation to implementors, and
advice to clients that some property often but doesn't necessarily hold.

> For example,
>
> 147 * <p>An <i>unmodifiable collection</i> is a collection, all of whose
> 148 * mutator methods (as defined above) are specified to throw
> 149 * {@code UnsupportedOperationException}. Such a collection thus cannot be
> 150 * modified by calling any methods on it. For a collection to be properly
> 151 * unmodifiable, any view collections derived from it *must *also be
> unmodifiable.
> 152 * For example, if a List is unmodifiable, the List returned by
> 153 * {@link List#subList List.subList} *should *also be unmodifiable.
>
> That leaves room for a non-compliant collection which could not technically be
> called "unmodifiable".

I agree that "should" is poor here; I'll reword this to say that the returned
list "is also unmodifiable."

> 138 * does not support the operation. Such methods *should *(but are not required
> 139 * to) throw an {@code UnsupportedOperationException} if the invocation would
> 140 * have no effect on the collection. For example, invoking the
> 141 * {@link #addAll addAll} method on a collection that does not support
> 142 * the {@link #add add} operation *should *throw the exception if
> 143 * the collection passed as an argument is empty.
>
> These two sentences both allow not throwing UOE and requiring UOE. Can they be
> worded to be consistent (or similarly qualified.)

For this case, implementations are recommended to throw UOE, but they are not
required to do so. Some implementations will throw, while others will not throw.
There are examples of both in the JDK. I'll clarify the example, but I really do
mean "should" here.

> Set.java: 706:  Would using "unique elements" clarify the contents of the result?
> Saying "Duplicate elements are permitted," may be misleading.
> Perhaps "The given Collection may contain duplicate elements"...

I'll clarify this to say that if the given Collection contains duplicate
elements, an arbitrary element of the duplicates is preserved.

> Similarly in Collectors.to toUnmodifiableSet:
> "Duplicate elements are allowed," can be misleading, especially the word allowed.

Will update similarly.

> Collectors.toUnmodifiableMap
>       * If the mapped keys
> 1474      * might have duplicates, use {@link #toUnmodifiableMap(Function,
> Function, BinaryOperator)}
> 1475      * instead.
>
> It would helpful to say why to use the version with the BinaryOperator, for
> example to say"
> "To determine which of the duplicate elements to retain".  or similar.

Will add words about handling the merging of values.

> Collectors:1606:  impl note:  With three different NPE throws, it is helpful to
> use the 2-arg
> form of requireNonNull to indicate which parameter offends.

Will update.

Thanks,

s'marks

>
> Roger
>
> On 10/30/2017 6:50 PM, Stuart Marks wrote:
>> (also includes 8184690: add Collectors for collecting into unmodifiable List,
>> Set, and Map)
>>
>> Hi all,
>>
>> Here's an updated webrev for this changeset; the previous review thread is here:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>
>> This webrev includes the following:
>>
>> * specification revisions to provide clearer definitions of "view"
>> collections, "unmodifiable" collections, and "unmodifiable views"
>>
>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>
>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>
>> * tests for the new API methods
>>
>> I've added some assertions that require some independence between the source
>> collection (or map) and the result of the copyOf() method.
>>
>> I've made a small but significant change to Set.copyOf compared to the
>> previous round. Previously, it specified that the first of any equal elements
>> was preserved. Now, it is explicitly unspecified which of any equals elements
>> is preserved. This is consistent with Set.addAll, Collectors.toSet, and the
>> newly added Collectors.toUnmodifiableSet, none of which specify which of
>> duplicate elements is preserved.
>>
>> (The outlier here is Stream.distinct, which specifies that the first element
>> of any duplicates is preserved, if the stream is ordered.)
>>
>> I've also made some minor wording/editorial changes in response to suggestions
>> from David Holmes and Roger Riggs. I've kept the wording changes that give
>> emphasis to "unmodifiable" over "immutable." The term "immutable" is
>> inextricably intertwined with "persistent" when it comes to data structures,
>> and I believe we'll be explaining this forever if Java's "immutable" means
>> something different from everybody else's.
>>
>> Webrev:
>>
>>     http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>
>> Bugs:
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8177290
>>         add copy factory methods for unmodifiable List, Set, Map
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8184690
>>         add Collectors for collecting into unmodifiable List, Set, and Map
>>
>> Thanks,
>>
>> s'marks
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stuart Marks
In reply to this post by Stuart Marks
Updated webrev, based on comments from Brian and Roger:

     http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/

s'marks


On 10/30/17 3:50 PM, Stuart Marks wrote:

> (also includes 8184690: add Collectors for collecting into unmodifiable List,
> Set, and Map)
>
> Hi all,
>
> Here's an updated webrev for this changeset; the previous review thread is here:
>
>      
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>
> This webrev includes the following:
>
> * specification revisions to provide clearer definitions of "view" collections,
> "unmodifiable" collections, and "unmodifiable views"
>
> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>
> * new Collectors.toUnmodifiableList, Set, and Map methods
>
> * tests for the new API methods
>
> I've added some assertions that require some independence between the source
> collection (or map) and the result of the copyOf() method.
>
> I've made a small but significant change to Set.copyOf compared to the previous
> round. Previously, it specified that the first of any equal elements was
> preserved. Now, it is explicitly unspecified which of any equals elements is
> preserved. This is consistent with Set.addAll, Collectors.toSet, and the newly
> added Collectors.toUnmodifiableSet, none of which specify which of duplicate
> elements is preserved.
>
> (The outlier here is Stream.distinct, which specifies that the first element of
> any duplicates is preserved, if the stream is ordered.)
>
> I've also made some minor wording/editorial changes in response to suggestions
> from David Holmes and Roger Riggs. I've kept the wording changes that give
> emphasis to "unmodifiable" over "immutable." The term "immutable" is
> inextricably intertwined with "persistent" when it comes to data structures, and
> I believe we'll be explaining this forever if Java's "immutable" means something
> different from everybody else's.
>
> Webrev:
>
>      http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>
> Bugs:
>
>      https://bugs.openjdk.java.net/browse/JDK-8177290
>          add copy factory methods for unmodifiable List, Set, Map
>
>      https://bugs.openjdk.java.net/browse/JDK-8184690
>          add Collectors for collecting into unmodifiable List, Set, and Map
>
> Thanks,
>
> s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Bernd Eckenfels-4
Having a List.of(List) copy constructor would save an additional array copy in the collector Which uses  (List<T>)List.of(list.toArray())

Gruss
Bernd
--
http://bernd.eckenfels.net
________________________________
From: core-libs-dev <[hidden email]> on behalf of Stuart Marks <[hidden email]>
Sent: Wednesday, November 1, 2017 12:49:56 AM
To: core-libs-dev
Subject: Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Updated webrev, based on comments from Brian and Roger:

     http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/

s'marks


On 10/30/17 3:50 PM, Stuart Marks wrote:

> (also includes 8184690: add Collectors for collecting into unmodifiable List,
> Set, and Map)
>
> Hi all,
>
> Here's an updated webrev for this changeset; the previous review thread is here:
>
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>
> This webrev includes the following:
>
> * specification revisions to provide clearer definitions of "view" collections,
> "unmodifiable" collections, and "unmodifiable views"
>
> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>
> * new Collectors.toUnmodifiableList, Set, and Map methods
>
> * tests for the new API methods
>
> I've added some assertions that require some independence between the source
> collection (or map) and the result of the copyOf() method.
>
> I've made a small but significant change to Set.copyOf compared to the previous
> round. Previously, it specified that the first of any equal elements was
> preserved. Now, it is explicitly unspecified which of any equals elements is
> preserved. This is consistent with Set.addAll, Collectors.toSet, and the newly
> added Collectors.toUnmodifiableSet, none of which specify which of duplicate
> elements is preserved.
>
> (The outlier here is Stream.distinct, which specifies that the first element of
> any duplicates is preserved, if the stream is ordered.)
>
> I've also made some minor wording/editorial changes in response to suggestions
> from David Holmes and Roger Riggs. I've kept the wording changes that give
> emphasis to "unmodifiable" over "immutable." The term "immutable" is
> inextricably intertwined with "persistent" when it comes to data structures, and
> I believe we'll be explaining this forever if Java's "immutable" means something
> different from everybody else's.
>
> Webrev:
>
>      http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>
> Bugs:
>
>      https://bugs.openjdk.java.net/browse/JDK-8177290
>          add copy factory methods for unmodifiable List, Set, Map
>
>      https://bugs.openjdk.java.net/browse/JDK-8184690
>          add Collectors for collecting into unmodifiable List, Set, and Map
>
> Thanks,
>
> s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Tagir Valeev
In reply to this post by Stuart Marks
Hello!

Set.of:

+        if (coll instanceof ImmutableCollections.AbstractImmutableSet) {
+            return (Set<E>)coll;
+        } else {
+            return (Set<E>)Set.of(coll.stream().distinct().toArray());

I think that good old Set.of(new HashSet<>(coll).toArray()) would
produce less garbage. distinct() also maintains HashSet internally,
but it removes the SIZED characteristic, so instead of preallocated
array you will have a SpinedBuffer which is less efficient than
AbstractCollection.toArray() implementation which just allocates the
array of exact size. What do you think?

Collectors:

+    static final Set<Collector.Characteristics> CH_UNORDERED_NOID
+            = Collections.unmodifiableSet(EnumSet.of(Collector.Characteristics.UNORDERED));

Is it really more efficient currently than
Set.of(Collector.Characteristics.UNORDERED)? At least less objects
will be allocated with Set.of

+    Collector<T, ?, List<T>> toUnmodifiableList() {
+        return new CollectorImpl<>((Supplier<List<T>>)
ArrayList::new, List::add,
+                                   (left, right) -> {
left.addAll(right); return left; },
+                                   list -> (List<T>)List.of(list.toArray()),
+                                   CH_NOID);
+    }

Isn't it reasonable to use `e -> List.add(Objects.requireNonNull(e))`
instead of simply `List::add`? In this case if null is added, then
failure will occur much earlier, and the failure stacktrace would be
more relevant. The same for Set/Map.

+                map ->
(Map<K,U>)Map.ofEntries(map.entrySet().toArray(new Map.Entry[0])));

It's the same lambda in two versions of toUnmodifiableMap. Isn't it
better to extract it to the constant to prevent duplication in the
bytecode (or at least to method and refer to it via method reference)?

With best regards,
Tagir Valeev.




With best regards,
Tagir Valeev.

On Wed, Nov 1, 2017 at 12:49 AM, Stuart Marks <[hidden email]> wrote:

> Updated webrev, based on comments from Brian and Roger:
>
>     http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/
>
> s'marks
>
>
>
> On 10/30/17 3:50 PM, Stuart Marks wrote:
>>
>> (also includes 8184690: add Collectors for collecting into unmodifiable
>> List, Set, and Map)
>>
>> Hi all,
>>
>> Here's an updated webrev for this changeset; the previous review thread is
>> here:
>>
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>
>> This webrev includes the following:
>>
>> * specification revisions to provide clearer definitions of "view"
>> collections, "unmodifiable" collections, and "unmodifiable views"
>>
>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>
>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>
>> * tests for the new API methods
>>
>> I've added some assertions that require some independence between the
>> source collection (or map) and the result of the copyOf() method.
>>
>> I've made a small but significant change to Set.copyOf compared to the
>> previous round. Previously, it specified that the first of any equal
>> elements was preserved. Now, it is explicitly unspecified which of any
>> equals elements is preserved. This is consistent with Set.addAll,
>> Collectors.toSet, and the newly added Collectors.toUnmodifiableSet, none of
>> which specify which of duplicate elements is preserved.
>>
>> (The outlier here is Stream.distinct, which specifies that the first
>> element of any duplicates is preserved, if the stream is ordered.)
>>
>> I've also made some minor wording/editorial changes in response to
>> suggestions from David Holmes and Roger Riggs. I've kept the wording changes
>> that give emphasis to "unmodifiable" over "immutable." The term "immutable"
>> is inextricably intertwined with "persistent" when it comes to data
>> structures, and I believe we'll be explaining this forever if Java's
>> "immutable" means something different from everybody else's.
>>
>> Webrev:
>>
>>      http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>
>> Bugs:
>>
>>      https://bugs.openjdk.java.net/browse/JDK-8177290
>>          add copy factory methods for unmodifiable List, Set, Map
>>
>>      https://bugs.openjdk.java.net/browse/JDK-8184690
>>          add Collectors for collecting into unmodifiable List, Set, and
>> Map
>>
>> Thanks,
>>
>> s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stuart Marks
In reply to this post by Bernd Eckenfels-4
On 10/31/17 5:52 PM, Bernd Eckenfels wrote:
> Having a List.of(List) copy constructor would save an additional array copy in the collector Which uses  (List<T>)List.of(list.toArray())

The quickest way to get all the elements from the source collection is to call
toArray() on it. Some copy constructors (like ArrayList's) simply use the
returned array for internal storage. This saves a copy, but it relies on the
source collection's toArray() implementation to be correct. In particular, the
returned array must be freshly created, and that array must be of type Object[].
If either of these is violated, it will break the ArrayList.

The "immutable" collections behind List.of/copyOf/etc. are fastidious about
allocating their internal arrays in order to ensure that they are referenced
only from within the newly created collection. This requires making an "extra"
copy of the array returned by toArray().

An alternative is for the new collection to preallocate its internal array using
the source's size(), and then to copy the elements out. But the source's size
might change during the copy (e.g., if it's a concurrent collection) so this
complicates things.

I think the only safe way to avoid the extra copy is to create a private
interface that can be used by JDK implementations.

s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Patrick Reinhart

> Am 01.11.2017 um 20:25 schrieb Stuart Marks <[hidden email]>:
>
> On 10/31/17 5:52 PM, Bernd Eckenfels wrote:
>> Having a List.of(List) copy constructor would save an additional array copy in the collector Which uses  (List<T>)List.of(list.toArray())
>
> The quickest way to get all the elements from the source collection is to call toArray() on it. Some copy constructors (like ArrayList's) simply use the returned array for internal storage. This saves a copy, but it relies on the source collection's toArray() implementation to be correct. In particular, the returned array must be freshly created, and that array must be of type Object[]. If either of these is violated, it will break the ArrayList.
>
> The "immutable" collections behind List.of/copyOf/etc. are fastidious about allocating their internal arrays in order to ensure that they are referenced only from within the newly created collection. This requires making an „extra" copy of the array returned by toArray().
>
> An alternative is for the new collection to preallocate its internal array using the source's size(), and then to copy the elements out. But the source’s
> size might change during the copy (e.g., if it’s a concurrent collection) so this complicates things.

I think the array „overhead“ would be only for the cases of zero, one and two value implementations. That seems to me not worth of optimizing…

> I think the only safe way to avoid the extra copy is to create a private interface that can be used by JDK implementations.
>
> s'marks

-Patrick
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Louis Wasserman
I disagree, actually.  Collections with size zero and one are significantly
more common than bigger collections.

In Guava's immutable collection factories (ImmutableList.of(...) etc.), we
observed a roughly exponential decline in the number of users of factory
methods of each size: if N people created empty lists, ~N/2 created
singleton lists, ~N/4 created two-element lists, etc.  We got noticeable
pushback from RAM-sensitive customers when we eliminated some specialized
singleton collection implementations.

Our experience has been that specializing for N=0 and N=1 does pay off.
Probably not N=2, though?

On Wed, Nov 1, 2017 at 12:45 PM Patrick Reinhart <[hidden email]> wrote:

>
> > Am 01.11.2017 um 20:25 schrieb Stuart Marks <[hidden email]>:
> >
> > On 10/31/17 5:52 PM, Bernd Eckenfels wrote:
> >> Having a List.of(List) copy constructor would save an additional array
> copy in the collector Which uses  (List<T>)List.of(list.toArray())
> >
> > The quickest way to get all the elements from the source collection is
> to call toArray() on it. Some copy constructors (like ArrayList's) simply
> use the returned array for internal storage. This saves a copy, but it
> relies on the source collection's toArray() implementation to be correct.
> In particular, the returned array must be freshly created, and that array
> must be of type Object[]. If either of these is violated, it will break the
> ArrayList.
> >
> > The "immutable" collections behind List.of/copyOf/etc. are fastidious
> about allocating their internal arrays in order to ensure that they are
> referenced only from within the newly created collection. This requires
> making an „extra" copy of the array returned by toArray().
> >
> > An alternative is for the new collection to preallocate its internal
> array using the source's size(), and then to copy the elements out. But the
> source’s
> > size might change during the copy (e.g., if it’s a concurrent
> collection) so this complicates things.
>
> I think the array „overhead“ would be only for the cases of zero, one and
> two value implementations. That seems to me not worth of optimizing…
>
> > I think the only safe way to avoid the extra copy is to create a private
> interface that can be used by JDK implementations.
> >
> > s'marks
>
> -Patrick
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Patrick Reinhart
In this case I would prefer a non static copyOf() method on the list to create a unmodifiable list/set/map, where the optimal factory method can be called. This would also solve the problem of a concurrent implementation.

-Patrick


> Am 01.11.2017 um 21:05 schrieb Louis Wasserman <[hidden email]>:
>
> I disagree, actually.  Collections with size zero and one are significantly more common than bigger collections.
>
> In Guava's immutable collection factories (ImmutableList.of(...) etc.), we observed a roughly exponential decline in the number of users of factory methods of each size: if N people created empty lists, ~N/2 created singleton lists, ~N/4 created two-element lists, etc.  We got noticeable pushback from RAM-sensitive customers when we eliminated some specialized singleton collection implementations.
>
> Our experience has been that specializing for N=0 and N=1 does pay off.  Probably not N=2, though?
>
> On Wed, Nov 1, 2017 at 12:45 PM Patrick Reinhart <[hidden email] <mailto:[hidden email]>> wrote:
>
> > Am 01.11.2017 um 20:25 schrieb Stuart Marks <[hidden email] <mailto:[hidden email]>>:
> >
> > On 10/31/17 5:52 PM, Bernd Eckenfels wrote:
> >> Having a List.of(List) copy constructor would save an additional array copy in the collector Which uses  (List<T>)List.of(list.toArray())
> >
> > The quickest way to get all the elements from the source collection is to call toArray() on it. Some copy constructors (like ArrayList's) simply use the returned array for internal storage. This saves a copy, but it relies on the source collection's toArray() implementation to be correct. In particular, the returned array must be freshly created, and that array must be of type Object[]. If either of these is violated, it will break the ArrayList.
> >
> > The "immutable" collections behind List.of/copyOf/etc. are fastidious about allocating their internal arrays in order to ensure that they are referenced only from within the newly created collection. This requires making an „extra" copy of the array returned by toArray().
> >
> > An alternative is for the new collection to preallocate its internal array using the source's size(), and then to copy the elements out. But the source’s
> > size might change during the copy (e.g., if it’s a concurrent collection) so this complicates things.
>
> I think the array „overhead“ would be only for the cases of zero, one and two value implementations. That seems to me not worth of optimizing…
>
> > I think the only safe way to avoid the extra copy is to create a private interface that can be used by JDK implementations.
> >
> > s'marks
>
> -Patrick

Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

roger riggs
In reply to this post by Stuart Marks
Hi Stuart,

A few editorial comments:

Collection.java: Lines 110, 133, 166
The bold labels probably want to be on their own lines and not
terminated by "." to look like headings
(or be headings if the CSS supports them)

List.java: Consistency of markup references to unmodifiable List|Set|Map.
The List.of constructors put the reference on a separate line, but the
copyOf constructor
does not.  You could probably omit the blank line.
(BTW, the copyOf constructor does not always create a copy; I'm not sure
if the method
name will result in an incorrect assumption but it may be misleading or
a spec issue.)

The same observations are true for Map and Set constructors.

Thanks, Roger




On 10/31/2017 7:49 PM, Stuart Marks wrote:

> Updated webrev, based on comments from Brian and Roger:
>
>     http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/
>
> s'marks
>
>
> On 10/30/17 3:50 PM, Stuart Marks wrote:
>> (also includes 8184690: add Collectors for collecting into
>> unmodifiable List, Set, and Map)
>>
>> Hi all,
>>
>> Here's an updated webrev for this changeset; the previous review
>> thread is here:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>
>> This webrev includes the following:
>>
>> * specification revisions to provide clearer definitions of "view"
>> collections, "unmodifiable" collections, and "unmodifiable views"
>>
>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory"
>> methods
>>
>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>
>> * tests for the new API methods
>>
>> I've added some assertions that require some independence between the
>> source collection (or map) and the result of the copyOf() method.
>>
>> I've made a small but significant change to Set.copyOf compared to
>> the previous round. Previously, it specified that the first of any
>> equal elements was preserved. Now, it is explicitly unspecified which
>> of any equals elements is preserved. This is consistent with
>> Set.addAll, Collectors.toSet, and the newly added
>> Collectors.toUnmodifiableSet, none of which specify which of
>> duplicate elements is preserved.
>>
>> (The outlier here is Stream.distinct, which specifies that the first
>> element of any duplicates is preserved, if the stream is ordered.)
>>
>> I've also made some minor wording/editorial changes in response to
>> suggestions from David Holmes and Roger Riggs. I've kept the wording
>> changes that give emphasis to "unmodifiable" over "immutable." The
>> term "immutable" is inextricably intertwined with "persistent" when
>> it comes to data structures, and I believe we'll be explaining this
>> forever if Java's "immutable" means something different from
>> everybody else's.
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>
>> Bugs:
>>
>>      https://bugs.openjdk.java.net/browse/JDK-8177290
>>          add copy factory methods for unmodifiable List, Set, Map
>>
>>      https://bugs.openjdk.java.net/browse/JDK-8184690
>>          add Collectors for collecting into unmodifiable List, Set,
>> and Map
>>
>> Thanks,
>>
>> s'marks

Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stefan Zobel
In reply to this post by Stuart Marks
Hi Stuart,

only a minor nit. In Map.java, the class-level
Javadoc anchor id="immutable" doesn't match the
href="#unmodifiable" used in the methods.

+ * <h2><a id="immutable">Unmodifiable Maps</a></h2>

vs.

+ * See <a href="#unmodifiable">Unmodifiable Maps</a> for details.


Regards,
Stefan


2017-11-01 0:49 GMT+01:00 Stuart Marks <[hidden email]>:

> Updated webrev, based on comments from Brian and Roger:
>
>     http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/
>
> s'marks
>
>
>
> On 10/30/17 3:50 PM, Stuart Marks wrote:
>>
>> (also includes 8184690: add Collectors for collecting into unmodifiable
>> List, Set, and Map)
>>
>> Hi all,
>>
>> Here's an updated webrev for this changeset; the previous review thread is
>> here:
>>
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>
>> This webrev includes the following:
>>
>> * specification revisions to provide clearer definitions of "view"
>> collections, "unmodifiable" collections, and "unmodifiable views"
>>
>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>
>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>
>> * tests for the new API methods
>>
>> I've added some assertions that require some independence between the
>> source collection (or map) and the result of the copyOf() method.
>>
>> I've made a small but significant change to Set.copyOf compared to the
>> previous round. Previously, it specified that the first of any equal
>> elements was preserved. Now, it is explicitly unspecified which of any
>> equals elements is preserved. This is consistent with Set.addAll,
>> Collectors.toSet, and the newly added Collectors.toUnmodifiableSet, none of
>> which specify which of duplicate elements is preserved.
>>
>> (The outlier here is Stream.distinct, which specifies that the first
>> element of any duplicates is preserved, if the stream is ordered.)
>>
>> I've also made some minor wording/editorial changes in response to
>> suggestions from David Holmes and Roger Riggs. I've kept the wording changes
>> that give emphasis to "unmodifiable" over "immutable." The term "immutable"
>> is inextricably intertwined with "persistent" when it comes to data
>> structures, and I believe we'll be explaining this forever if Java's
>> "immutable" means something different from everybody else's.
>>
>> Webrev:
>>
>>      http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>
>> Bugs:
>>
>>      https://bugs.openjdk.java.net/browse/JDK-8177290
>>          add copy factory methods for unmodifiable List, Set, Map
>>
>>      https://bugs.openjdk.java.net/browse/JDK-8184690
>>          add Collectors for collecting into unmodifiable List, Set, and
>> Map
>>
>> Thanks,
>>
>> s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stuart Marks
In reply to this post by Tagir Valeev


On 11/1/17 10:45 AM, Tagir Valeev wrote:

> Set.of:
>
> +        if (coll instanceof ImmutableCollections.AbstractImmutableSet) {
> +            return (Set<E>)coll;
> +        } else {
> +            return (Set<E>)Set.of(coll.stream().distinct().toArray());
>
> I think that good old Set.of(new HashSet<>(coll).toArray()) would
> produce less garbage. distinct() also maintains HashSet internally,
> but it removes the SIZED characteristic, so instead of preallocated
> array you will have a SpinedBuffer which is less efficient than
> AbstractCollection.toArray() implementation which just allocates the
> array of exact size. What do you think?

Oh yes, good point. I had initially used stream().distinct() because I wanted to
use distinct()'s semantics of preserving the first equal element among
duplicates. But since I removed that requirement from the spec, using a HashSet
as you suggest is much simpler.

> Collectors:
>
> +    static final Set<Collector.Characteristics> CH_UNORDERED_NOID
> +            = Collections.unmodifiableSet(EnumSet.of(Collector.Characteristics.UNORDERED));
>
> Is it really more efficient currently than
> Set.of(Collector.Characteristics.UNORDERED)? At least less objects
> will be allocated with Set.of

Maybe. I'm just following the same pattern as the nearby characteristics.
They're singletons, so this hardly makes any difference. What's really needed is
an unmodifiable EnumSet, which I hope to get to at some point.

> +    Collector<T, ?, List<T>> toUnmodifiableList() {
> +        return new CollectorImpl<>((Supplier<List<T>>)
> ArrayList::new, List::add,
> +                                   (left, right) -> {
> left.addAll(right); return left; },
> +                                   list -> (List<T>)List.of(list.toArray()),
> +                                   CH_NOID);
> +    }
>
> Isn't it reasonable to use `e -> List.add(Objects.requireNonNull(e))`
> instead of simply `List::add`? In this case if null is added, then
> failure will occur much earlier, and the failure stacktrace would be
> more relevant. The same for Set/Map.

Interesting. Initally I thought this would be a good idea, but then I tried it
out. With the implementation in my latest webrev, the stack trace is this:


jshell> Arrays.asList(1, 2, null,
4).stream().collect(Collectors.toUnmodifiableList())
|  java.lang.NullPointerException thrown
|        at Objects.requireNonNull (Objects.java:221)
|        at ImmutableCollections$ListN.<init> (ImmutableCollections.java:234)
|        at List.of (List.java:1039)
|        at Collectors.lambda$toUnmodifiableList$6 (Collectors.java:299)
|        at ReferencePipeline.collect (ReferencePipeline.java:515)
|        at (#2:1)


With the lambda and requireNonNull(), the stack trace is this:


jshell> Arrays.asList(1, 2, null,
4).stream().collect(Collectors.toUnmodifiableList())
|  java.lang.NullPointerException thrown
|        at Objects.requireNonNull (Objects.java:221)
|        at Collectors.lambda$toUnmodifiableList$5 (Collectors.java:298)
|        at ReduceOps$3ReducingSink.accept (ReduceOps.java:169)
|        at Spliterators$ArraySpliterator.forEachRemaining (Spliterators.java:948)
|        at AbstractPipeline.copyInto (AbstractPipeline.java:484)
|        at AbstractPipeline.wrapAndCopyInto (AbstractPipeline.java:474)
|        at ReduceOps$ReduceOp.evaluateSequential (ReduceOps.java:913)
|        at AbstractPipeline.evaluate (AbstractPipeline.java:234)
|        at ReferencePipeline.collect (ReferencePipeline.java:511)
|        at (#2:1)


It's true that with lambda+requireNonNull, the NPE will be thrown earlier in
processing. But the stack trace isn't any clearer (the only user code is at the
very bottom at location "#2:1") and it's still within the context of the same
call to the stream's terminal collect() call. So, doesn't seem like it makes
much difference.


> +                map ->
> (Map<K,U>)Map.ofEntries(map.entrySet().toArray(new Map.Entry[0])));
>
> It's the same lambda in two versions of toUnmodifiableMap. Isn't it
> better to extract it to the constant to prevent duplication in the
> bytecode (or at least to method and refer to it via method reference)?

It might be better to do that, but given that this is the code I want to replace
with a private interface, I don't think it's worth fiddling around with at this
point.

Thanks,

s'marks


> With best regards,
> Tagir Valeev.
>
>
>
>
> With best regards,
> Tagir Valeev.
>
> On Wed, Nov 1, 2017 at 12:49 AM, Stuart Marks <[hidden email]> wrote:
>> Updated webrev, based on comments from Brian and Roger:
>>
>>      http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/
>>
>> s'marks
>>
>>
>>
>> On 10/30/17 3:50 PM, Stuart Marks wrote:
>>>
>>> (also includes 8184690: add Collectors for collecting into unmodifiable
>>> List, Set, and Map)
>>>
>>> Hi all,
>>>
>>> Here's an updated webrev for this changeset; the previous review thread is
>>> here:
>>>
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>>
>>> This webrev includes the following:
>>>
>>> * specification revisions to provide clearer definitions of "view"
>>> collections, "unmodifiable" collections, and "unmodifiable views"
>>>
>>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>>
>>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>>
>>> * tests for the new API methods
>>>
>>> I've added some assertions that require some independence between the
>>> source collection (or map) and the result of the copyOf() method.
>>>
>>> I've made a small but significant change to Set.copyOf compared to the
>>> previous round. Previously, it specified that the first of any equal
>>> elements was preserved. Now, it is explicitly unspecified which of any
>>> equals elements is preserved. This is consistent with Set.addAll,
>>> Collectors.toSet, and the newly added Collectors.toUnmodifiableSet, none of
>>> which specify which of duplicate elements is preserved.
>>>
>>> (The outlier here is Stream.distinct, which specifies that the first
>>> element of any duplicates is preserved, if the stream is ordered.)
>>>
>>> I've also made some minor wording/editorial changes in response to
>>> suggestions from David Holmes and Roger Riggs. I've kept the wording changes
>>> that give emphasis to "unmodifiable" over "immutable." The term "immutable"
>>> is inextricably intertwined with "persistent" when it comes to data
>>> structures, and I believe we'll be explaining this forever if Java's
>>> "immutable" means something different from everybody else's.
>>>
>>> Webrev:
>>>
>>>       http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>>
>>> Bugs:
>>>
>>>       https://bugs.openjdk.java.net/browse/JDK-8177290
>>>           add copy factory methods for unmodifiable List, Set, Map
>>>
>>>       https://bugs.openjdk.java.net/browse/JDK-8184690
>>>           add Collectors for collecting into unmodifiable List, Set, and
>>> Map
>>>
>>> Thanks,
>>>
>>> s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stuart Marks
In reply to this post by roger riggs


On 11/1/17 1:50 PM, Roger Riggs wrote:
> Collection.java: Lines 110, 133, 166
> The bold labels probably want to be on their own lines and not terminated by
> "." to look like headings
> (or be headings if the CSS supports them)
I'll change these to be actual headings.
> List.java: Consistency of markup references to unmodifiable List|Set|Map.
> The List.of constructors put the reference on a separate line, but the copyOf
> constructor
> does not.  You could probably omit the blank line.
Yeah, I should update all of these at some point. Given that there are 30-odd
more methods to change, plus I have a pile of other collections doc changes to
work on, I think I'll do these in a separate doc pass.
> (BTW, the copyOf constructor does not always create a copy; I'm not sure if
> the method
> name will result in an incorrect assumption but it may be misleading or a spec
> issue.)
Right. I haven't been able to come up with any names that had the right
semantics and that didn't also have connotations that were misleading in some
other way. I observe that Guava's Immutable collections have copyOf() methods
with pretty much the same semantics. The Guava docs do note that their methods
try to avoid making a copy if they can, but they explicitly say that the
circumstances under which this occurs are unspecified. I've considered adding an
@apiNote to this effect, but I haven't been able to convince myself that it
would be helpful to do so. It would seem to raise new issues that we're
unwilling to answer, such as exactly when is a copy is made vs. when the
argument is returned. Better, I think, to have people make a copy whenever they
think they need a copy, and have the implementation short-circuit this when it can.

s'marks

>
> The same observations are true for Map and Set constructors.
>
> Thanks, Roger
>
>
>
>
> On 10/31/2017 7:49 PM, Stuart Marks wrote:
>> Updated webrev, based on comments from Brian and Roger:
>>
>> http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/
>>
>> s'marks
>>
>>
>> On 10/30/17 3:50 PM, Stuart Marks wrote:
>>> (also includes 8184690: add Collectors for collecting into unmodifiable
>>> List, Set, and Map)
>>>
>>> Hi all,
>>>
>>> Here's an updated webrev for this changeset; the previous review thread is
>>> here:
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>>
>>> This webrev includes the following:
>>>
>>> * specification revisions to provide clearer definitions of "view"
>>> collections, "unmodifiable" collections, and "unmodifiable views"
>>>
>>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>>
>>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>>
>>> * tests for the new API methods
>>>
>>> I've added some assertions that require some independence between the source
>>> collection (or map) and the result of the copyOf() method.
>>>
>>> I've made a small but significant change to Set.copyOf compared to the
>>> previous round. Previously, it specified that the first of any equal
>>> elements was preserved. Now, it is explicitly unspecified which of any
>>> equals elements is preserved. This is consistent with Set.addAll,
>>> Collectors.toSet, and the newly added Collectors.toUnmodifiableSet, none of
>>> which specify which of duplicate elements is preserved.
>>>
>>> (The outlier here is Stream.distinct, which specifies that the first element
>>> of any duplicates is preserved, if the stream is ordered.)
>>>
>>> I've also made some minor wording/editorial changes in response to
>>> suggestions from David Holmes and Roger Riggs. I've kept the wording changes
>>> that give emphasis to "unmodifiable" over "immutable." The term "immutable"
>>> is inextricably intertwined with "persistent" when it comes to data
>>> structures, and I believe we'll be explaining this forever if Java's
>>> "immutable" means something different from everybody else's.
>>>
>>> Webrev:
>>>
>>> http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>>
>>> Bugs:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8177290
>>>          add copy factory methods for unmodifiable List, Set, Map
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8184690
>>>          add Collectors for collecting into unmodifiable List, Set, and Map
>>>
>>> Thanks,
>>>
>>> s'marks
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stuart Marks
In reply to this post by Stefan Zobel
Good catch! Thanks, I'll fix it.

s'marks

On 11/1/17 4:01 PM, Stefan Zobel wrote:

> Hi Stuart,
>
> only a minor nit. In Map.java, the class-level
> Javadoc anchor id="immutable" doesn't match the
> href="#unmodifiable" used in the methods.
>
> + * <h2><a id="immutable">Unmodifiable Maps</a></h2>
>
> vs.
>
> + * See <a href="#unmodifiable">Unmodifiable Maps</a> for details.
>
>
> Regards,
> Stefan
>
>
> 2017-11-01 0:49 GMT+01:00 Stuart Marks <[hidden email]>:
>> Updated webrev, based on comments from Brian and Roger:
>>
>>      http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.2/
>>
>> s'marks
>>
>>
>>
>> On 10/30/17 3:50 PM, Stuart Marks wrote:
>>>
>>> (also includes 8184690: add Collectors for collecting into unmodifiable
>>> List, Set, and Map)
>>>
>>> Hi all,
>>>
>>> Here's an updated webrev for this changeset; the previous review thread is
>>> here:
>>>
>>>
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049261.html
>>>
>>> This webrev includes the following:
>>>
>>> * specification revisions to provide clearer definitions of "view"
>>> collections, "unmodifiable" collections, and "unmodifiable views"
>>>
>>> * new List.copyOf(), Set.copyOf(), and Map.copyOf() "copy factory" methods
>>>
>>> * new Collectors.toUnmodifiableList, Set, and Map methods
>>>
>>> * tests for the new API methods
>>>
>>> I've added some assertions that require some independence between the
>>> source collection (or map) and the result of the copyOf() method.
>>>
>>> I've made a small but significant change to Set.copyOf compared to the
>>> previous round. Previously, it specified that the first of any equal
>>> elements was preserved. Now, it is explicitly unspecified which of any
>>> equals elements is preserved. This is consistent with Set.addAll,
>>> Collectors.toSet, and the newly added Collectors.toUnmodifiableSet, none of
>>> which specify which of duplicate elements is preserved.
>>>
>>> (The outlier here is Stream.distinct, which specifies that the first
>>> element of any duplicates is preserved, if the stream is ordered.)
>>>
>>> I've also made some minor wording/editorial changes in response to
>>> suggestions from David Holmes and Roger Riggs. I've kept the wording changes
>>> that give emphasis to "unmodifiable" over "immutable." The term "immutable"
>>> is inextricably intertwined with "persistent" when it comes to data
>>> structures, and I believe we'll be explaining this forever if Java's
>>> "immutable" means something different from everybody else's.
>>>
>>> Webrev:
>>>
>>>       http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.1/
>>>
>>> Bugs:
>>>
>>>       https://bugs.openjdk.java.net/browse/JDK-8177290
>>>           add copy factory methods for unmodifiable List, Set, Map
>>>
>>>       https://bugs.openjdk.java.net/browse/JDK-8184690
>>>           add Collectors for collecting into unmodifiable List, Set, and
>>> Map
>>>
>>> Thanks,
>>>
>>> s'marks
Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Peter Levart
In reply to this post by Stuart Marks
Hi,

On 11/02/2017 12:51 AM, Stuart Marks wrote:

> On 11/1/17 10:45 AM, Tagir Valeev wrote:
>> Set.of:
>>
>> +        if (coll instanceof
>> ImmutableCollections.AbstractImmutableSet) {
>> +            return (Set<E>)coll;
>> +        } else {
>> +            return (Set<E>)Set.of(coll.stream().distinct().toArray());
>>
>> I think that good old Set.of(new HashSet<>(coll).toArray()) would
>> produce less garbage. distinct() also maintains HashSet internally,
>> but it removes the SIZED characteristic, so instead of preallocated
>> array you will have a SpinedBuffer which is less efficient than
>> AbstractCollection.toArray() implementation which just allocates the
>> array of exact size. What do you think?
>
> Oh yes, good point. I had initially used stream().distinct() because I
> wanted to use distinct()'s semantics of preserving the first equal
> element among duplicates. But since I removed that requirement from
> the spec, using a HashSet as you suggest is much simpler.

Why not using:

     coll.stream().collect(Collectors.toImmutableSet())

As Collectors.toImmutableSet() is currently implemented, with serial
Stream it will create a single HashSet, add all the elements to it and
call Set.of(HashSet.toArray()) with it. Pretty much the same as what
Tagir proposes, but the Collector could be made more efficient in the
future and with it, the optimization would automatically extend to
Set.copyOf()...

Regards, Peter

Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

roger riggs
In reply to this post by Patrick Reinhart
Hi,

It would also support a more fluent use of the APIs.  Though if we start
down that road
it may become littered with convenience methods.  Something to ponder
for a bit.

Roger


On 11/1/2017 4:29 PM, Patrick Reinhart wrote:

> In this case I would prefer a non static copyOf() method on the list to create a unmodifiable list/set/map, where the optimal factory method can be called. This would also solve the problem of a concurrent implementation.
>
> -Patrick
>
>
>> Am 01.11.2017 um 21:05 schrieb Louis Wasserman <[hidden email]>:
>>
>> I disagree, actually.  Collections with size zero and one are significantly more common than bigger collections.
>>
>> In Guava's immutable collection factories (ImmutableList.of(...) etc.), we observed a roughly exponential decline in the number of users of factory methods of each size: if N people created empty lists, ~N/2 created singleton lists, ~N/4 created two-element lists, etc.  We got noticeable pushback from RAM-sensitive customers when we eliminated some specialized singleton collection implementations.
>>
>> Our experience has been that specializing for N=0 and N=1 does pay off.  Probably not N=2, though?
>>
>> On Wed, Nov 1, 2017 at 12:45 PM Patrick Reinhart <[hidden email] <mailto:[hidden email]>> wrote:
>>
>>> Am 01.11.2017 um 20:25 schrieb Stuart Marks <[hidden email] <mailto:[hidden email]>>:
>>>
>>> On 10/31/17 5:52 PM, Bernd Eckenfels wrote:
>>>> Having a List.of(List) copy constructor would save an additional array copy in the collector Which uses  (List<T>)List.of(list.toArray())
>>> The quickest way to get all the elements from the source collection is to call toArray() on it. Some copy constructors (like ArrayList's) simply use the returned array for internal storage. This saves a copy, but it relies on the source collection's toArray() implementation to be correct. In particular, the returned array must be freshly created, and that array must be of type Object[]. If either of these is violated, it will break the ArrayList.
>>>
>>> The "immutable" collections behind List.of/copyOf/etc. are fastidious about allocating their internal arrays in order to ensure that they are referenced only from within the newly created collection. This requires making an „extra" copy of the array returned by toArray().
>>>
>>> An alternative is for the new collection to preallocate its internal array using the source's size(), and then to copy the elements out. But the source’s
>>> size might change during the copy (e.g., if it’s a concurrent collection) so this complicates things.
>> I think the array „overhead“ would be only for the cases of zero, one and two value implementations. That seems to me not worth of optimizing…
>>
>>> I think the only safe way to avoid the extra copy is to create a private interface that can be used by JDK implementations.
>>>
>>> s'marks
>> -Patrick

Reply | Threaded
Open this post in threaded view
|

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

Stuart Marks
In reply to this post by Peter Levart

> Why not using:
>
>     coll.stream().collect(Collectors.toImmutableSet())
>
> As Collectors.toImmutableSet() is currently implemented, with serial Stream it
> will create a single HashSet, add all the elements to it and call
> Set.of(HashSet.toArray()) with it. Pretty much the same as what Tagir
> proposes, but the Collector could be made more efficient in the future and
> with it, the optimization would automatically extend to Set.copyOf()...
This is mainly about whether Set.copyOf() is implemented in terms of
Collectors.toUnmodifiableSet(), or vice-versa, which then calls Set.of(T[]) to
do the actual creation. Some future optimization will probably replace both of
these implementations with calls to JDK internal methods that can bypass the
extra copying, so it doesn't really matter which one of these calls the other
right now.

s'marks

12