hg: jdk8/tl/jdk: 8008167: IdentityHashMap.[keySet|values|entrySet].toArray speed-up

[Mike Duigou]

I have created an issue for fixing the introduced regression and created a regression test that would have caught the error (fix a bug, write a test...).

8008785: IdentityHashMap.values().toArray(V[]) broken by JDK-8008167

http://cr.openjdk.java.net/~mduigou/JDK-8008785/0/webrev

Mike

On Feb 24 2013, at 12:27 , Peter Levart wrote:

> Hi Mike,
> 
> I thought I saw that too when you commited the change, but then re-examinig the whole source in detail, I couldn't spot it again. I must have stared at the wrong third of change...
> 
> Regards, Peter
> 
> On 02/24/2013 07:53 PM, Mike Duigou wrote:
>> Ouch, this would have been introduced by me.
>> 
>> I will check to see how this could have passed the pre-commit regression testing. I suspect that a regression test needs to be improved.
>> 
>> Mike
>> 
>> On Feb 24 2013, at 10:48 , Alan Bateman wrote:
>> 
>>> On 24/02/2013 15:49, Peter Levart wrote:
>>>> Hi Alan,
>>>> 
>>>> I checked and it seems all 3 IHM views [keySet|values|entrySet] have a fail-fast iterator implementation (IdentityHashMap.IdentityHashMapIterator) and all 3 are (were) using the iterator for .toArray implementations. So this patch tries to preserve the behavior when there is a concurrent modification (which is only possible from other thread and is illegal usage anyway since IHM is not thread-safe) while executing the toArray methods on the views...
>>>> 
>>>> Do you see something I don't see?
>>> My apologies, I see it does check the modification count in IdentityHashMapIterator.nextIndex.
>>> 
>>> However, as this forced me to looks at the changes-set again then the copy loop in Values.toArray has caught by eye:
>>> 
>>>             for (int si = 0; si < tab.length; si += 2) {
>>>                 if (tab[si++] != null) { // key present ?
>>>                     // more elements than expected -> concurrent modification from other thread
>>>                     if (ti >= size) {
>>>                         throw new ConcurrentModificationException();
>>>                     }
>>>                     a[ti++] = (T) tab[si]; // copy value
>>>                 }
>>>             }
>>> 
>>> Looks like si is incrementing by 3 rather than 2 (which ironically will cause a CME later because there will be fewer elements copied than expected). 
>>> 
>>> Do you concur? If so then we can create a bug to change this to test tab[si] and copy in tab[si+1].
>>> 
>>> -Alan
>