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

[Sean Chou]

Hi Ulf,
    I hope following comments can help reduce your concern.

On Thu, Mar 15, 2012 at 6:27 PM, Ulf Zibis <Ulf.Zibis at gmx.de> wrote:

> 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.

I don't think this footprint is a problem until it is proved to be one. If
it is, a better javac can be used to save this footprint, and it would save
much more. The actual problem might be reached by
http://openjdk.java.net/jeps/149 .



>
>  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.

A comment just help understand the code, does not remove this thinking.


>
>

>
>> 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.
>>
> 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.

It is nice, I had written a version of toArray(T[] a) which put all the
elements directly into array a, and allocate a new array only when there
are more elements than a can contain. However, 7153238 is a RFE and this is
a bug. I would like the bug to be fixed in a way easy to understand if
possible. And you can put all the enhancement in the RFE which in fact
would do much more then this piece of byte code footprint saving. It is
better to do one thing in one bug/RFE .


>
>  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.

The comment"// Check less elements" and " // Check equal elements" clearly
describe the two blocks of code are testing different scenarios.
And a new definition would emphasize it is a different variable, it is used
to test different scenarios.


>
>
>  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.

Do you mean ConcurrentHashMap returns a collection does not inherit
AbstractCollection ? Don't worry about that, ConcurrentHashMap is a class
of java collection framework, it won't let that
happen. AbstractCollection is born to be inherited, especially by
collection classes in java collection framework.



>
>
>  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/~zhouyx/7121314/webrev.02/>
>>        <http://cr.openjdk.java.net/%**7Ezhouyx/7121314/webrev.02/<http://cr.openjdk.java.net/%7Ezhouyx/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
>>
>>


-- 
Best Regards,
Sean Chou