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

[Mike Duigou]

On Mar 26 2011, at 01:10 , Neil Richards wrote:

> Hi Mike - thanks for posting up the webrev, and for your review.
> 
> On Fri, 2011-03-25 at 19:33 -0700, Mike Duigou wrote:
>> 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. 
> 
> With the assignment line suitably corrected, I believe there is no need
> for the added check.
> Please find below a corrected changeset 'hg export' with the relevant
> correction made.

I will apply this patch and retest. 

>> 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.
> 
> Our versions of Java 6 (which have been "out in the field" for more than
> 3 years now) are not susceptible to this particular problem, so our
> performance folk (yes, we have them too!) have obviously found its
> resolution to be unconcerning.
> 
> Whilst developing the suggested change, I also reviewed EnumMap and
> IdentityHashMap for opportunities to avoid using entry sets (and their
> iterators) for the current object altogether.
> This resulted in the expanded methods I highlighted in my previous post.
> (In these cases, the performance should actually improve slightly, as
> using the expansion avoids at least two object creations).
> 
> Do you have any particular "common benchmarks" in mind?
> I'm currently out of office for a few days (visiting family in Concord),
> but can look to work on provide supporting data once I'm back "in situ",
> as necessary.

My concern was/is just a general anxiety in that I reasonably expect that the improved version will be slightly slower but I can't make a definitive assertion that the slowdown is insignificant.

I did two very quick tests
  - One exercises the iterator but completes before there is a need to GC. There was essentially no difference in performance. 
  - The other text starts a timer task that does a full GC every half second. Again there was no difference in performance.

Mike