RFR: jsr166 jdk integration 2018-05

[Martin Buchholz]

On Wed, May 16, 2018 at 2:21 PM, Stuart Marks <stuart.marks at 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....