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

[Stuart Marks]

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 <stuart.marks at oracle.com> 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