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

[Aleksey Shipilev]

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.


>> 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.

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.


> 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.

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.


> 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.


Thanks,
-Aleksey.