RFR: 8001647: In-place methods on Collection/List (original) (raw)

David Holmes david.holmes at oracle.com
Fri Apr 19 05:29:01 PDT 2013

Hi Akhil,

This is only a partial review so far.

I have some issues with the ConcurrentModificationException strategy.

Taking ArrayList, the basic idea is that you want to detect modifications that are concurrent with iteration - so the mutators up the modCount and the iterator functions check to see if the modCount has changed since the iterator was constructed. So you've then treated the bulk operations as "iterations" and you check the modCount each time through the loop. Problem is the modCount is not volatile so in all likelihood the JIT is going to hoist the check of modCount out of the loop and it will only be checked once. That's not incorrect as it is "best effort" detection, but it might be surprising. (Note if existing iterator methods get inlined the same problem can exist today.) Maybe it is not worth trying to check this? The reality is that if the ArrayList is really being modified concurrently - particularly if the size changes

It's late and I may have overlooked ssomething but in Vector all the methods are synchronized yet you still check the modCount for changes ?? But it is impossible for anyone to change modCount during a bullk operation. It is only possible with iteration because a mutating method in one thread can execute between the separate iterator hasNext()/next() calls in another thread. So all the CME code can be dropped from the new bulk methods.

Cheers, David

On 19/04/2013 4:49 AM, Akhil Arora wrote:

Looks like the stars are aligning on getting on this into TL... the refreshed webrev is -

http://cr.openjdk.java.net/~akhil/8001647.8/webrev/ Please review Thanks On 12/10/2012 09:31 PM, Akhil Arora wrote: http://cr.openjdk.java.net/~akhil/8001647.3/webrev/

- now with synchronized and unmodifiable wrappers in Collections.java for the default methods being added On 12/10/2012 01:48 PM, Akhil Arora wrote: Updated with yours and Alan's comments -

http://cr.openjdk.java.net/~akhil/8001647.2/webrev/ - removed null check for removeSet - cache this.size in removeAll, replaceAll (for ArrayList, Vector and CopyOnWriteArrayList) - calculate removeCount instead of BitCount.cardinality() - removed unnecessary @library from test support classes

More information about the lambda-dev mailing list