RFR: jsr166 jdk integration 2018-05

[Paul Sandoz]

> On May 11, 2018, at 9:33 AM, Martin Buchholz <martinrb at google.com> wrote:
> 
> 
> 
> On Fri, May 11, 2018 at 9:06 AM, Paul Sandoz <paul.sandoz at oracle.com <mailto:paul.sandoz at oracle.com>> wrote:
> 
> 
>> On May 9, 2018, at 11:17 AM, Martin Buchholz <martinrb at google.com <mailto:martinrb at google.com>> wrote:
>> 
>> Time to do this, since Claes is also touching ArrayList.
>> 
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html <http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html>
>> 
>> 8202685: Improve ArrayList replaceAll
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/replaceAll/index.html <http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/replaceAll/index.html>
>> https://bugs.openjdk.java.net/browse/JDK-8202685 <https://bugs.openjdk.java.net/browse/JDK-8202685>
>> 
> 
> ArrayList.java
> —
>       @Override
>       public void replaceAll(UnaryOperator<E> operator) {
>           Objects.requireNonNull(operator);
>           final int expectedModCount = modCount;
>           final Object[] es = elementData;
> !         final int size = this.size;
> !         for (int i = 0; modCount == expectedModCount && i < size; i++)
>               es[i] = operator.apply(elementAt(es, i));
>           if (modCount != expectedModCount)
>               throw new ConcurrentModificationException();
> -         modCount++;
>       }
> 
> A CME is not necessarily associated with just structural modifications it could, on a best effort basis, be associated with any modification, which is cheaper to do for bulk operations rather than individual operations, and this operation can be used to perturb all the elements of the list (just like sort) causing strange and hard to track bugs while in the middle of iterating. IMHO its better to fail under such circumstances rather than be silent.
> 
> It's tempting to treat modCount as a count of ALL modifications, especially given its name, but that's different from the historical behavior and design of these classes.  Consistency with existing spec and implementations is the biggest argument. 

I mentally revised the history when doing the collections/stream API work since we added more bulk operations, since this is on a “best effort” basis and if it’s cheap to do then there is no real harm in it and it might help.


> You can argue that the very idea of structural modifications was a bad idea, but it's still there in https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/util/List.html <https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/util/List.html>  
> 

> (Personally, I think checking for concurrent modification at all is not worth the small improvement in safety)

I tend to agree, the cost of specifying and implementing is quite high (i have been saved once by this functionality).


>  
> 
>> 8201386: Miscellaneous changes imported from jsr166 CVS 2018-05
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html <http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html>
>> https://bugs.openjdk.java.net/browse/JDK-8201386 <https://bugs.openjdk.java.net/browse/JDK-8201386>
>> 
> 
> 
> 
> PriorityQueue
> —
> 
>  587     public E poll() {
>  588         final Object[] es;
>  589         final E result;
>  590 
>  591         if ((result = (E) ((es = queue)[0])) != null) {
>  592             modCount++;
>  593             final int n;
>  594             final E x = (E) es[(n = --size)];
>  595             es[n] = null;
>  596             if (n > 0) {
>  597                 final Comparator<? super E> cmp;
>  598                 if ((cmp = comparator) == null)
>  599                     siftDownComparable(0, x, es, n);
>  600                 else
>  601                     siftDownUsingComparator(0, x, es, n, cmp);
>  602             }
>  603         }
>  604         return result;
>  605     }
> 
> Why inline the siftDown method?
> 
> There is an effort here to remove gratuitous implementation differences between PriorityQueue and PriorityBlockingQueue.  Maybe this one should go the other way - refactor the corresponding code in PBQ into siftDown. But can we trust the VM to not re-read the queue and size fields?

If it inlines it probably won’t :-)

Paul.