Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec (original) (raw)

Ulf Zibis Ulf.Zibis at gmx.de
Tue Mar 13 23:36:02 UTC 2012

Am 13.03.2012 07:58, schrieb Sean Chou:

Hi Ulf and David,

I modified the patch and added the testcase, it's now : http://cr.openjdk.java.net/~zhouyx/7121314/webrev.02/ <http://cr.openjdk.java.net/%7Ezhouyx/7121314/webrev.02/> . Ulf's compact version is used, it looks beautiful; Thanks!

however I replaced the Math.min part with if statement because if statement is more intuitive and I don't think there is any performance concern. But it is not so compact now... My performance thoughts: Your version: } else if (a.length < i) { // dereferences a.length ... } else { // push original i to stack System.arraycopy(r, 0, a, 0, i); // references array elements, uses some CPU registers if (a.length > i) { // dereferences a.length again, pop i from stack a[i] = null; // null-termination // references array elements again

better: } else if (a.length < i) { // dereferences a.length ... } else { if (a.length > i) { // reuses a.length result + i from above i++; // ensure null-termination // cheap operation System.arraycopy(r, 0, a, 0, i); // references array elements, original i must not be remembered

compact: } else if (a.length < i) { ... } else { System.arraycopy(r, 0, a, 0, a.length > i ? ++i : i); // ensure null-termination

Note: I first used Math.min() as it should be intrinsified by JIT, but above seems faster again.

Comment maybe more intuitive/clear: 197 return it.hasNext() ? // more elements than expected 198 finishToArray(r, it) : r;

Please additionally note my alternative comment at bug 7153238 <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153238> - Use smaller more optimistic array size increase for AbstractCollection.toArray()

Also I added the equal size case and @author to testcase. You can reuse variable map instead map2, otherwise maybe confusing why saving map from 1st testcase.

Add comments: 44 remove(keys[0]); // simulate concurrent decrease of map's elements 54 remove(keys[0]); // simulate concurrent decrease of map's elements 67 Object[] res = map.values().toArray(a); // inherits from AbstractCollection.toArray() 77 res = map2.values().toArray(a); // inherits from AbstractCollection.toArray()

Your test does not cover cases: if (a == r) if (a.length < i) it.hasNext() ? finishToArray(r, it) : r (neither yes nor no)

There is a little problem when I created the webrev, I don't know how to change the "contributed-by" information for the testcase, so the list is still Ulf's and my emails. Thanks listing me :-)

-Ulf

More information about the core-libs-dev mailing list