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.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 >
