RFR: jsr166 jdk9 integration wave 12

[Paul Sandoz]

Hi,

The j.u.c. changes look ok to me as do the misc changes.

The ArrayDeque changes may also change the performance characteristics, a space/time trade-off e.g. in current version array bounds checks are strength reduced to a zero-based test of the array length. Unsure in practice if this really matters.

At this stage in Java 9 i would be inclined not to make ArrayDeque more ArrayList-like with new public methods ensureCapacity/trimToSize/replaceAll. Do you mind if we hold off an that for now and think more about that?

Testing-wise there is always gonna be some overlap. It would be nice to consolidate, although arguably in principle TCK has a slightly different focus. Now that the tck is in the OpenJDK repo perhaps we should add that to the jdk_collections_core?

Paul.

> On 18 Oct 2016, at 10:15, Stuart Marks <stuart.marks at oracle.com> wrote:
> 
> 
> 
> 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
>