RFR: jsr166 jdk9 integration wave 12

[Stuart Marks]

On 10/17/16 6:34 PM, Martin Buchholz wrote:
> Most of this is for Stuart - very collection-y.
>
> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
>
> This includes a rewrite of ArrayDeque to bring it to parity with ArrayList
> (except for List features).
> The patch includes public methods ensureCapacity, trimToSize, replaceAll as in
> ArrayList, but we can defer making them public to a later change (or a later
> release), since new public methods are always controversial.  But I'd really
> like to get the performance/scalability changes in for jdk 9.
>
> It also includes a redo of ArrayList#removeIf to make it more efficient and
> consistent with behavior of other implementations, including ArrayDeque.
>
> The patches are a little tangled because they step on each other's toes.  File
> CollectionTest is in "miscellaneous", but it tests both the ArrayDeque and
> ArrayList changes.

Hi Martin,

ArrayList.removeIf() is nice. I like the way it recovers from the predicate 
having thrown an exception. I note that it uselessly copies the tail of the 
array to itself, if the predicate throws an exception and nothing has been 
deleted yet. You could add a r != w check, or possibly deleted > 0 if you 
prefer, or maybe we don't care because this is a rare (we hope) error recovery case.

***

I have some comments on ArrayDeque:

* Change in allocation/capacity policy.

The removal of the power-of-two restriction, and applying a 1.5x growth factor 
(same as ArrayList) seems sensible. Does this mean that the ability to compute 
the proper array index by using x & (length-1) wasn't worth it? Or at least not 
worth the extra tail wastage?

(Potential future enhancement: allocate the array lazily, like ArrayList)

Note, I haven't digested all the changes yet.

* API additions

I'm somewhat undecided about these.

I've always felt that the ensureCapacity() and trimToSize() methods were a sop 
added to ArrayList aimed at people converting from Vector. The 
ArrayList.ensureCapacity() method seems to be used a lot, but probably not to 
great effect, since addAll() gives exact sizing anyway. The trimToSize() method 
could be useful in some cases, but should we hold out for a generalized one, as 
requested by JDK-4619094?

On the other hand, ArrayDeque is modeling an array, so providing some size 
control seems mostly harmless.

The replaceAll() method is a bit oddly placed here. It's a default method on 
List (which ArrayDeque doesn't, and can't implement) so it's a bit strange to 
have a special method here with the same name and semantics as the one on List, 
but with a "fork" of the specification.

Maybe if JDK-8143850 is implemented to give a list view of an ArrayDeque, the 
replaceAll() method could be implemented on that:

     arrayDeque.asList().replaceAll(clazz::method);

***

I'm not sure what to think of the arrangement of the tests. The "core" 
collections, that is, collections in java.util, exclusive of 
java.util.concurrent, are mostly tested by tests in jdk/test/java/util. This 
patch adds tests for ArrayList and ArrayDeque inside of 
jdk/test/java/util/concurrent/tck. There's a test group jdk_collections_core 
that's intended to test the core collections, so it'll miss the new tests being 
added here.

I'm not sure what to suggest. The new tests use the JSR166TestCase framework, 
which is probably useful, and probably can't be used if the tests are moved 
elsewhere. I don't know what it would take to convert this to jtreg or testng. 
Or, the core test group could be modified to run selected JSR166 test cases, 
which doesn't seem quite right either. Suggestions?

***

I haven't looked at the other j.u.c changes. I haven't done so historically, but 
I can if you need a reviewer.

s'marks