Collections.addAll: remove outdated performance advice and add a note about atomicity

[Stuart Marks]

Links to existing material in OpenJDK:

     http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050326.html

     https://bugs.openjdk.java.net/browse/JDK-8193031

I agree with the removal of the performance advice. We should also remove 
"identical"; my suggested replacement was "as if".

We can even add some hedging about whether the operation is atomic for certain 
collections (i.e., the synchronized ones). However, I'm not sure if there is 
anything useful to say about atomicity of bulk updates. They're only atomic for 
the synchronized collections, which are largely disused. It's pointless to talk 
about atomicity for non-concurrent collections, and the operations aren't atomic 
for things like CopyOnWriteArrayList and a Set projection of ConcurrentHashMap. 
So I'm not sure discussion of atomicity is useful or warranted here.

(Also, adding a collection of elements one at a time to a CopyOnWriteArrayList: 
*shudder*.)

I think the most useful thing is to define a new array-reading default method. 
Each implementation can then override it to use the best technique for that 
implementation. (I had previously called this "addEach" but I'm flexible on naming.)

Adding a new default method is kind of far afield from where this started. If 
the spec is bothersome, perhaps we could consider a spec cleanup change 
separately from implementation changes and definition of a new default method.

s'marks




On 1/30/18 7:07 PM, Martin Buchholz wrote:
> I tried to tackle this here:
> http://openjdk.markmail.org/thread/eet2zd6ig3pfpv5g
> and it's still on my TODO list but not likely to get to top spot soon.
> 
> On Tue, Jan 30, 2018 at 7:00 PM, Tagir Valeev <amaembo at gmail.com> wrote:
> 
>> Hello!
>>
>> I suggest a patch for java.util.Collections#addAll JavaDoc:
>>
>> --- Collections.java    2018-01-31 09:39:31.599107500 +0700
>> +++ Collections.java.patched    2018-01-31 09:51:11.929059600 +0700
>> @@ -5406,4 +5406,8 @@
>>        * The behavior of this convenience method is identical to that of
>> -     * {@code c.addAll(Arrays.asList(elements))}, but this method is
>> likely
>> -     * to run significantly faster under most implementations.
>> +     * {@code c.addAll(Arrays.asList(elements))} except possible
>> +     * difference in intermediate state visibility for concurrent or
>> +     * synchronized collections. Calling this method does not guarantee
>> +     * that the intermediate state (some of elements are added) is
>> invisible,
>> +     * even if the collection itself provides such guarantee for its
>> +     * {@link Collection#addAll(Collection)} method.
>>        *
>>
>> First, currently it says that Collections#addAll is likely to run
>> significantly faster. However it's only marginally faster for
>> collections which delegate their addAll method to standard
>> AbstractCollection#addAll implementation. Also it could be much slower
>> for collections which have optimized addAll (like ArrayList,
>> CopyOnWriteArrayList, ConcurrentLinkedDeque, etc.). I don't know a
>> single example of collection where Collections#addAll is actually
>> significantly faster. Also it says that the behavior is identical,
>> while it's not. If, e.g. c is a collection returned from
>> synchronizedCollection, then intermediate state of
>> c.addAll(Arrays.asList(elements)) would not be visible under
>> synchronized(c) in another thread. On the other hand, replacing such
>> call with Collections.addAll(c, elements) (to make it "significantly
>> faster") will lift this guarantee: now you can see partially added
>> array.
>>
>> What do you think? Should I file an issue?
>>
>> With best regards,
>> Tagir Valeev.
>>