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 


> On 18 Oct 2016, at 10:15, Stuart Marks <stuart.ma...@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

Reply via email to