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. 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.
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<http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153238>> says not available to me. About the testcase, variable map and map2 are of different types, can not be reused. I would not like to add "// inherits from AbstractCollection.toArray()" , it is obvious and listed in java doc. 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. 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 . On Wed, Mar 14, 2012 at 7:36 AM, Ulf Zibis <ulf.zi...@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/~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 <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
