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
