RFR: 8001647: In-place methods on Collection/List

[David Holmes]

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 
- then you will just get exceptions (NPE, AIOOBE etc) not a nice neat CME.

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
>