RFR: JDK-8035663 Suspicious failure of test java/util/concurrent/Phaser/FickleRegister.java

[David Holmes]

On 19/11/2014 10:44 PM, Aleksey Shipilev wrote:
> On 11/19/2014 03:03 PM, David Holmes wrote:
>> On 19/11/2014 9:21 PM, Aleksey Shipilev wrote:
>> The lock provides the atomicity needed for the logical CAS operation.
>> Atomic loads and stores don't help with that. But atomic loads and
>> stores are needed to ensure direct assignment of the volatile field
>> (which is already implemented via Atomic::store) can't interfere with a
>> concurrent "cas" operation.
>
> I understand the intent now. I guess our saving grace is that the
> absence of NATIVE_CX8 precludes the use of intrinsics or other codegen
> tricks that bypass the current Unsafe stubs.

Yes. Also I should have given a clearer picture of the context. This 
part of the Unsafe API is not intended for general use and mixing with 
regular Java code. It is only used as the implementation for a few of 
the java.util.concurrent classes. It isn't even used by 
AtomicLongFieldUpdater (which implements the locking at the Java level 
directly using the updater instance as the lock object). As per the test 
name the problem usage here was exposed via Phaser - which uses Unsafe 
directly for performance - to access its "private volatile long state" 
field.

>
>>> Given the interaction with Java code, I wonder if this should be handled
>>> in per-platform Atomic::* definitions, not in Unsafe stubs?
>>
>> Not sure what you mean by this, but this is the implementation of the
>> Unsafe API - which is done using the runtime facilities of the Atomic
>> class. This code handles both the supports_cx8 and doesn't_support_cx8
>> cases.
>
> This is my point: we leak the platform-specific details into Unsafe API
> stubs. I think this contributes to technical debt. Implementing the
> locked Atomic::cmpxchg(jlong) for doesn't_support_cx8 case still seems
> more fitting. Your excellent comment in unsafe.cpp really belongs in
> global Atomic definitions.

And it could have been implemented that way from the beginning but was not.

> This also becomes a more correct fix: if compilers inject runtime call
> to Atomic::load/store for volatile long fields when NATIVE_CX8=false,
> then they would properly serialize with lock-protected CAS.

Normal accesses to volatile long fields are already atomic as per the 
requirements of the spec. They are atomic wrt each other but not wrt 
these lock-based operations. The 'correct' fix is not to make direct 
accesses serialize with the lock-protected CAS, (which would kill the 
performance of non-CASing Java code) but to not mix such accesses in the 
first place. Again this is not an application coding issue but a library 
implementation issue.

>
>> The aim here is not to try to provide an Atomic::cmpxchg(jlong)
>> implementation for general use, but to provide the atomic operations
>> needed by the Unsafe class. We've deliberately steered away from
>> using a 64-bit CAS in the runtime precisely because it is not
>> available on all platforms, and a lock-based solution for the general
>> runtime would become a bottleneck. Of course if the original
>> implementation had instead gone down that path we wouldn't have known
>> anything different - but it didn't.
>
> Why? jlong atomicity on that particular platform does not seem an
> isolated and one-off Unsafe issue. This does seem like a generic problem
> you will face on that particular platform in future.

Simple fact is that not all 32-bit platforms support a 64-bit CAS. So we 
have avoided implementing anything in the VM runtime that requires a 
64-bit CAS on a 32-bit platform. If we implemented the 
Atomic::cmpxchg(jlong) for all platforms as you suggest then people 
would have used it without too much thought - potentially leading to 
problems in key algorithms on platforms without suports_cx8. We've 
actually caught this happening a few times over recent years. When it 
does we question whether the variable in question really needs to be 
64-bit on a 32-bit platform and the answer so far has always been no - 
so we either use int, or intptr_t so that we get 32-bit on 32-bit and 
64-bit on 64-bit.

> It feels weird to steer away from implementing the Atomic APIs because
> "it can become a bottleneck". That is, you may want to steer away from
> *using* the APIs excessively if you know it has problems on some
> platforms. But steering away from *implementing* leads to finally
> implementing it in ad-hoc places, e.g. Unsafe stubs in this case.

It is not as general as you make out. We only need 64-bit CAS in very 
limited places.

>
>> This patch simply fixes the current broken implementation of Unsafe.
>
> I agree with the approach to dodge the deadlock, but if we touch this
> area of the runtime code, it seems a good idea to make it more
> consistent and less spread-out.

Touching the code to fix the existing bugs does not mean we should 
re-design and implement this part of the runtime support.

Thanks for looking at it.

David

>
> Thanks,
> -Aleksey.
>