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

[Mikael Vidstedt]

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?

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

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
>