RFR(S): 8150921: Update Unsafe getters/setters to use double-register variants

[David Holmes]

On 4/05/2016 6:17 AM, Mikael Vidstedt wrote:
>
>
> On 5/3/2016 1:05 PM, Aleksey Shipilev wrote:
>> On 05/03/2016 09:43 PM, Mikael Vidstedt wrote:
>>> On 5/3/2016 8:15 AM, Aleksey Shipilev wrote:
>>>> *) Why do we Handle-ize incoming object in CompareAnd*? Is this because
>>>> we may acquire a mutex, and the object may be gone? Either way, I see
>>>> you have dropped Handle from Unsafe_CompareAndSwapLong, but kept it in
>>>> Unsafe_CompareAndExchangeLong -- why?
>>>>
>>>> *) Ditto for old {Get|Set}LongVolatile -- it used to Handle-ize before
>>>> acquiring a mutex -- now we don't do that in MemoryAccess.
>>> My bad. As you say resolving the object should happen after the mutex
>>> has been acquired (see long comment starting on line 377 for the
>>> background). I have added back the Handle in CompareAndSwapLong and
>>> moved the addr() call in the mutex methods of MemoryAccess. Thank you
>>> for catching that!
>> Right. We have two related problems:
>>
>>   a) the code in Unsafe_{Get|Set}LongVolatile computes the address
>> *before* acquiring the mutex, contrary to what comment suggests for
>> safety; this is fixed by your patch -- MemoryAccess handles that;
>>
>>   b) the code in Unsafe_CompareAnd{Exchange,Swap}Long computes the
>> address *before* acquiring the mutex, and this is still problematic; I
>> wonder if you want to soak in CAS/CAE into MemoryAccess too?
>>
>> I actually wonder why those accessors (especially non-long) are
>> UNSAFE_ENTRY, not UNSAFE_LEAF, if "we can treat those as leaf
>> routines"...
>
> Ah, yes, now I see what you're saying. In general, since the mutex isn't
> actually checking for safepoints right now this isn't really a problem,
> but it would definitely be nice to unify it. How about I file a separate
> enhancement for moving the other memory operations into MemoryAccess?

I need to look at this again to see what I previously changed it from 
and to. No safepoints means no object movement so calculating before the 
lock acquisition is safe. That does seem to make the use of Handles 
unnecessary though ...

>> Another question to pile on: why only LongVolatile is treated specially,
>> but not DoubleVolatile too? Atomicity requirements are the same for both
>> long and double.
>
> Very good question. I don't have the answer, but maybe somebody else knows.

For doubles we use FPU tricks to get atomicity for volatile accesses. 
The differences are historical. In theory we could have a platform that 
needs locking for volatile doubles too.

David
-----

> Cheers,
> Mikael
>
>>
>>>> vmSymbols.cpp:
>>>>
>>>> *) {get|put}Address_raw intrinsics used to be conditionalized under
>>>> UseUnalignedAccesses. Baffles me why, because the old native code in
>>>> Unsafe_{Get|Set}NativeAddress/unsafe.cpp would seem to fail with
>>>> misaligned addresses. I wonder if the intent was to never fail, and
>>>> then
>>>> we should really use Unsafe.{get|put}Unaligned in new Unsafe.java
>>>> {get|set}Address to dodge misalign faults?
>>> I also failed to see what the conditional intrinsification is trying to
>>> achieve given that the native code does nothing special for the
>>> unaligned case. Unless I'm missing something the proposed change makes
>>> it no worse, and will potentially improve performance a tad on the
>>> pickier platforms. If anything we may want to consider introducing
>>> methods for getAddressUnaligned and putAddressUnaligned as a separate
>>> change.
>> Yes, there is no advertised behavior for misaligned {get|put}Address.
>> Only the intrinsic filters trip you off. I agree we can move on from
>> there.
>>
>>> jdk:
>>> http://cr.openjdk.java.net/~mikael/webrevs/8150921/webrev.02/jdk/webrev/
>>> hotspot:
>>> http://cr.openjdk.java.net/~mikael/webrevs/8150921/webrev.02/hotspot/webrev/
>>>
>> Thanks,
>> -Aleksey
>>
>