[concurrency-interest] Fwd: Should I avoid compareAndSet with value-based classes?

[Michael Hixson]

Forwarding this reply of mine because I forgot to CC the mailing list.


---------- Forwarded message ----------
From: Michael Hixson <michael.hixson at gmail.com>
Date: Thu, Jul 6, 2017 at 7:24 AM
Subject: Re: [concurrency-interest] Should I avoid compareAndSet with
value-based classes?
To: Andrew Haley <aph at redhat.com>


On Thu, Jul 6, 2017 at 4:20 AM, Andrew Haley <aph at redhat.com> wrote:
> On 06/07/17 11:41, Alex Otenko wrote:
>>
>>> On 6 Jul 2017, at 10:13, Andrew Haley <aph at redhat.com> wrote:
>>>
>>> On 06/07/17 04:59, Michael Hixson wrote:
>>>> AtomicReference and VarHandle are specified to use == in compareAndSet
>>>> (and related) operations [1].  Using == to compare instances of
>>>> value-based classes may lead to "unpredictable results" [2].  Does
>>>> this mean I should avoid using compareAndSet with arguments that are
>>>> instances of value-based classes?
>>>>
>>>> It seems like the documentation clearly tells me "yes, avoid doing
>>>> that" but I'm hoping I misunderstood, or maybe AtomicReference and
>>>> VarHandle are exempt somehow.  Otherwise, how do I implement
>>>> non-broken compareAndSet and updateAndGet for a java.time.Instant
>>>> value for example?
>>>
>>> java.time.Instant stores times that are longer than a JVM word, so
>>> they cannot be CAS'd in a lock-free way unless a factory guarantees
>>> that instances which compare equal also have the property of
>>> reference equality.  j.t.Instant factories in the Java library are
>>> explicitly documented *not* to have this property, so that doesn't
>>> help.
>>
>> That’s not entirely clear.
>>
>> Wouldn’t this loop work:
>>
>> volatile j.t.Instant curr = ...
>>
>> j.t.Instant next = …
>> j.t.Instant tmp = ...
>> do {
>>   tmp = curr;
>>   if (tmp.equal(next)) break;
>> } while(!curr.CAS(tmp, next));
>>
>> // assume curr is equal to next
>
> Something like that, yes.  But it's going to be evil if there are
> several high-frequency writers.  If you're doing all that work in
> order to CAS a timestamp, why not use a synchronized block?  It would
> at least be less prone to the thundering herd, and we'd generate
> pretty decent code for that.  It'd be interesting to compare and
> contrast the two approaches for contended and non-contended cases.

The main reason I reached for AtomicReference is that I thought, "I
want thread-safe updates to this read-heavy value where writes won't
get lost if there's contention -- this sounds like the sort of problem
that java.util.concurrent.atomic solves."

As a minor point, I wanted synchronization on reads to be as
minor/invisible as possible, to affect the readers' behavior as little
as possible (in comparison to their behavior when the value they're
reading a constant value with no synchronization).

But if AtomicReference is simply the wrong tool to use here, I
shouldn't use it.  That's fine.

>
>>> If you want to be able to CAS a reference to a j.t.Instant, you're
>>> going to have to wrap accesses to it in a synchronized block.  This is
>>> a direct consequence of the JVM's inability to CAS multi-word objects.
>>
>> This is confusing.
>>
>> Surely this isn’t talking about CASing a reference? The contents of
>> the object can’t be assumed to have any atomicity properties,
>> whether it is j.t.Instant or not.
>
> I agree.  I'm trying to look at what the OP actually wants to do: I
> assume this is some kind of atomic timestamp, and the OP wants to be
> able to CAS an instance of j.u.Instant.

It was more of a general worry and question.  Should I ever use
AtomicReference with value-based classes?  It sounds like the answer
is no.

But here's some code that illustrates the specific problem I was
trying to solve.  I think it "works" right now, but that it's in
violation of the spec for value-based classes and so possibly not
future-proof, and I think I understand how to fix it now.  Thanks for
the tips.

    class MutableClock {

        static MutableClock create(Instant instant, ZoneId zone) {
            return new MutableClock(
                    new AtomicReference<>(instant),
                    zone);
        }

        private final AtomicReference<Instant> instantHolder;
        private final ZoneId zone;

        private MutableClock(
                AtomicReference<Instant> instantHolder,
                ZoneId zone) {
            this.instantHolder = instantHolder;
            this.zone = zone;
        }

        Instant instant() {
            return instantHolder.get();
        }

        ZoneId getZone() {
            return zone;
        }

        void setInstant(Instant newInstant) {
            instantHolder.set(newInstant);
        }

        void add(Duration amountToAdd) {
            // this is the part that uses == and CAS
            instantHolder.updateAndGet(
                  instant -> instant.plus(amountToAdd));
        }

        MutableClock withZone(ZoneId newZone) {
            // conveniently, AtomicReference also acts as a
            // vehicle for "shared updates" between instances
            // of my class
            return new MutableClock(instantHolder, newZone);
        }
    }

-Michael