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

[Ulf Zibis]

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