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.zi...@gmx.de
<mailto: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/%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