RFR JDK-8225339 Optimize HashMap.keySet()/HashMap.values()/HashSet toArray() methods

[Claes Redestad]

Hello Tagir,

On 2019-06-05 15:49, Tagir Valeev wrote:
> Hello, Claes!
> 
> Yes, it's true that CME will not be thrown anymore. Depending on the 
> concurrent modification kind one might face various ranges of incorrect 
> behavior, including the AIOOBE, NPE or incorrect array returned. As 
> Collection spec says [1]:
>  > In the absence of a stronger guarantee by the implementation, 
> undefined behavior may result from the invocation of any method on a 
> collection that is being mutated by another thread
> 
> In particular it's never mentioned in HashSet, HashMap or Collection 
> spec that toArray implements a fail-fast behavior: this is said only 
> about the iterator() method. 

that's not entirely true, since the @implSpec for toArray on
AbstractCollection states it is equivalent to iterating over the
collection[1].

So since the implementations you're changing inherit their current
implementation from AbstractCollection and the iterators of HashMap is
specified to be fail-fast, then that behavior can be argued to be
specified also for the affected toArray methods.

I'm not fundamentally objecting to the behavior change, but I do think
it needs careful review and a CSR (or at least plenty of reviewers
agreeing that one isn't needed).

> Also note that ArrayList and LinkedList 
> toArray implementation also never throws CME. LinkedHashSet.toArray in 
> my patch is pretty similar to LinkedList.toArray. So I feel that such 
> change is perfectly acceptable: toArray method has no moral obligation 
> to fail fast, and there's no reason why it should be more "robust" for 
> HashSet than for LinkedList. or ArrayList.
> 
> Finally I should note that there are even more unfortunate examples of 
> behavior during the concurrent modification. In particular, querying a 
> TreeMap element during the concurrent update may cause an infinite loop 
> which is usually much more unpleasant than random exception.

Using worse flaws elsewhere seems like a poor excuse to make. Shouldn't
it rather be turned into an argument in favor of fixing TreeMap...?

/Claes

[1] 
https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/util/AbstractCollection.html#toArray()