On Wed, May 16, 2018 at 2:21 PM, Stuart Marks <stuart.ma...@oracle.com>
wrote:
>
>
> On 5/15/18 8:02 PM, Martin Buchholz wrote:
>
>>
>> How many times the lock is acquired is an implementation detail, a
>> non-obvious tradeoff even.
>> vector.replaceAll holds the lock throughout, but vector.subList(0,
>> size).replaceAll holds the lock for each element (sigh ... racily (really a
>> bug!)).
>>
>
> In Vector's case it's specified, not an implementation detail. You can't
> perform certain bulk operations on a Vector without holding the lock
> externally.
>
What is specified? Vector is synchronized, but it's unspecified whether
external synchronization via synchronized (vector) {...} works
Vector.replaceAll simply inherits spec from List.replaceAll - it seems
unspecified whether bulk operations like replaceAll on Vector are atomic.
(In fact they are not atomic on the subList).
> Again, imagine this use case: there is a periodic background task
>> that
>> optimizes all the elements of a Vector
>> vector.replaceAll(x -> optimized(x))
>> That should not break any iterations in progress.
>>
>> I don't think it's possible to do that correctly without holding a
>> lock
>> around the entire iteration.
>>
>> I don't see why.
>>
>> If the lock is held, CME can't occur, as a concurrent replaceAll()
>> will
>> occur before or after the iteration, never in the middle.
>>
>> I don't see why - iteration is inherently non-atomic, so replaceAll could
>> be called in the middle.
>>
>> If an iteration over a Vector doesn't hold a lock, any
>> read-modify-write
>> operations (consider a loop with a ListIterator on which set() is
>> called)
>> can be interleaved with bulk operations (like replaceAll) which is
>> clearly
>> incorrect. In such cases, CME should be thrown.
>>
>> Imagine your iterators are all read-only, and don't care whether they see
>> an element or its replacement.
>>
>
> You're imagining an incredibly narrow use case. You're choosing Vector,
> not ArrayList; the replaceAll() operation must an optimization that doesn't
> affect the semantics of the object (say, the outcome of any logic); the
> iteration must be read-only, not read-modify-write; and the logic within
> the iteration "doesn't care" whether it gets old or new values.
>
> I don't find it compelling that it's possible to imagine such a case. Most
> code won't conform to it. And in fact it's really hard to tell whether it
> does.
>
> Consider an example like this:
>
> for (T t : vectorOfT) {
> print(t);
> }
>
> Suppose that a replaceAll() on another thread occurs, and that this is
> allowed. Does the application care whether the eventual printout contains
> partly new values and partly old values? How can you tell? It seems to me
> that this is more likely a programming error than a valid use case.
>
Vector is unloved and there are few good uses for it at all - the List
interface is concurrency-hostile. BUT one good use case for concurrency
here seems to be when there are no structural modifications, when an index
unambiguously identifies a "place". Sort of like AtomicReferenceArray. We
could aim towards making Vector and AtomicReferenceArray and
ConcurrentHashMap more useful and cross-compatible (e.g.
Vector.accumulateAndGet0
> Also, this use case cannot be written today, because CME is thrown.
>>
>> ?? Imagine there's only one writer thread, with some Iterating reader
>> threads. Every midnight, the writer thread optimizes all the elements
>> for (int i = 0; i < size; i++) vector.set(i, optimized(vector.get(i))
>> This code has worked well since the '90s without CME. One day the
>> maintainer notices shiny new Vector.replaceAll and "fixes" the code.
>> """It's perfectly safe""".
>> The CME is rare and so only caught when the maintainer has gone on to the
>> next job. The CMEs only happen at midnight!
>>
>
> By "cannot be written today" I mean that the following:
>
> for (T t: arrayList) {
> if (condition) {
> list.replaceAll();
> }
> }
>
> has ALWAYS thrown CME, since the introduction of replaceAll().
>
But that's a bug (!) and the variant below has never thrown CME, and I
don't want to compound the existing bug.
for (T t: arrayList) {
if (condition) {
list.subList(0, list.size()).replaceAll();
}
}
Sure, there are cases such as you describe where CME gets thrown only
> rarely. That's preferable to getting incorrect results equally rarely.
> That's the point of fail-fast.
>
> **
>
> (subsequent email)
>
> (We don't seem to be moving towards consensus ...)
>>
>
> Apparently....
>
> At the very least, having replaceAll increment modCount (inconsistently!)
>> is surprising to users and is not documented anywhere.
>>
>
> Maybe it should be documented then.
>
> **
>
> Why are you placing so much emphasis on *removing* the CME behavior? It's
> been this way since Java 8 was delivered. Is this causing a problem? What
> will be solved by removing it?
>
I started out "simply" optimizing ArrayList subList replaceAll
Then I noticed the obvious implementation would increment modCount.
Which would be introducing a new bug....
