Request for review: 6312706: Map entrySet iterators should return different entries on each call to next()

[Mike Duigou]

Hi Neil;

I have run the jtreg regression suite against your webrev. I had to make one change to IdentityHashMap to satisfy the Collection/MOAT test. The failure condition involved calling EntrySet.remove() when there was no current entry due to a prior remove() or because next() had never been called. Since the case was caught by the MOAT.java test I didn't update any of the other unit tests.

I have uploaded a webrev of the current version of the webrev to : 

http://cr.openjdk.java.net/~mduigou/6312706/0/webrev/

I am comfortable that the changes work as expected. I have some remaining concerns with possible impact on performance--not in general use; I've learned to expect that every data structure is abused by some benchmark in an atypical way that exposes the downside of any change. I will check this week with our performance SQE people to see if we can quantify the impact of this change. If you have any metrics that you can contribute which show this change to have negligible impact upon performance across common benchmarks it would definitely help to allay any fears.

Thanks,

Mike

On Mar 25 2011, at 11:37 , Mike Duigou wrote:

> I've started to run the standard tests on this patch to make sure everything works as expected.
> 
> I will try to have any other review notes by early next week.
> 
> Thanks,
> 
> Mike
> 
> On Mar 23 2011, at 17:51 , Neil Richards wrote:
> 
>> As previously trailed [1], please find below a suggested change to
>> address the problem reported in bug 6312706 [2], together with testcases
>> to demonstrate the veracity of the change.
>> 
>> The change causes the entry set iterators for java.util.EnumMap and
>> java.util.IdentityHashMap to return distinct Map.Entry objects for each
>> call to next().
>> 
>> As detailed in the bug report, by doing this, the entry set, its
>> iterator, and the returned Entry objects behave in the manner in which
>> one would expect them to, without weird unexpected edge conditions.
>> 
>> To mitigate the overhead of producing new objects for each Entry
>> returned, I've also expanded the implementation of EnumMap's equals(),
>> hashCode() and writeObject(), such that they avoid using its entry set
>> to traverse through the entries, and added suitable tests to demonstrate
>> these expanded forms still behave properly.
>> 
>> I've also added a testcase to demonstrate that the problem being fixed
>> here does not occur in java.util.concurrent.ConcurrentHashMap .
>> (It was suggested in the bug report that the problem also occurred
>> there, so I thought it worthwhile to include the testcase to demonstrate
>> that it doesn't).
>> 
>> As always, and comments / advice / suggestions gratefully received,
>> Thanks,
>> Neil
>> 
>> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-January/005763.html
>> [2] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6312706
>> 
>> -- 
>> Unless stated above:
>> IBM email: neil_richards at uk.ibm.com
>> IBM United Kingdom Limited - Registered in England and Wales with number 741598.
>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU