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

[Ulf Zibis]

Thanks for your comments too.

Am 14.03.2012 05:23, schrieb Sean Chou:
> Thanks for your comments, I have different opinions.
>
> About performance, I would like to say "this part of code is not in a path which causes 
> performance problem". In fact it should rarely execute, so there is no need to catch this little 
> optimization, and readability is more important.
Yes, I agree, but my 2nd motivation was to reduce the bytecode footprint a little, especially for 
such rarely executed code.

> With the if statement, it reads "put a null after the elements if there are more space", while 
> with your code, it reads "copy all the elements from r or copy all elements and 1 more from r if 
> there are more space" and we have to think "what's the next element in r ? ". In fact, we need 
> look back to find how r is defined "T[] r = a.length >= size ? a 
> : (T[])java.lang.reflect.Array.newInstance(a.getClass().getComponentType(), size);" and go through 
> the code once more to realize there is a null at that position.
Yes, a better comment would be necessary.

>
> And with JIT, will a.length and i be dereference/push/pop so many times or is it kept in cache or 
> in a register ? I believe it is better to forget the little possible enhancement here, which is 
> also encouraged by java. I'm sorry the page of bug 7153238 
> <http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153238>  says not available to me.
Please wait 1 more day, then the content should become publicly visible. I'm waiting too, as I have 
an additional comment in queue to correct a little error.

> About the testcase, variable map and map2 are of different types, can not be reused.
Oops, yes.
But you could reuse it (like Object[] res) with:
Map<String,Object> map = new TConcurrentHashMap<>(); // better: TCHM1
...
map = new TConcurrentHashMap2<>();
I agree, a little nit, but I had the other "missing" test cases in mind, where the numbering then 
could become confusing.

> I would not like to add "// inherits from AbstractCollection.toArray()" , it is obvious and listed 
> in java doc.
In ConcurrentHashMap.values:
     Overrides: values in class AbstractMap<K,V>
In AbstractMap.values:
     This implementation returns a collection that subclasses AbstractCollection.
But there is no guarantee, that the returned collection overrides or just delegates to 
AbstractCollection.toArray().
In worst case, t.j.u.AC.ToArray doesn't test j.u. AbstractCollection.toArray() at all.

> About "// simulate concurrent decrease of map's elements", it is not enough to describe clearly 
> why it can do that. People have to refer to the bug for more knowledge. So in my opinion, it 
> changes nothing. However, I can add it if you have a strong request.
IMO would be a help for people, later reviewing the test case.

> And about the cases the testcase doesn't cover.
> " if (a == r)
>    if (a.length < i)
>    it.hasNext() ? finishToArray(r, it) : r  (neither yes nor no)
> "
> This testcase is designed to check if the returned array is the given array. In the above 2 cases, 
> there is no need to do the check.
> if (a == r), of cause it is;  if (a.length < i), of cause it is not. This 2 cases will fail due to 
> more serious bug, not 7121314 .
My complain was in assumption, that a test, named t.j.u.AC.ToArray, should test the whole complexity 
of method toArray(), not just concerning a single bug.

-Ulf


>
>
> On Wed, Mar 14, 2012 at 7:36 AM, Ulf Zibis <Ulf.Zibis at gmx.de <mailto:Ulf.Zibis at gmx.de>> wrote:
>
>     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/>
>         <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
>
>
>
>
> -- 
> Best Regards,
> Sean Chou
>