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

[David Holmes]

On 4/05/2016 6:55 AM, David Holmes wrote:
>
>
> 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 ...

Arguably the Handles are no longer needed - but all these methods 
utilize them.

The locking choice was very specific as per the block comment:

// As all the locked-regions are very short and themselves non-blocking 
we can treat
// them as leaf routines and elide safepoint checks (ie we don't perform 
any thread
// state transitions even when blocking for the lock). Note that if we 
do choose to
// add safepoint checks and thread state transitions, we must ensure 
that we calculate
// the address of the field _after_ we have acquired the lock, else the 
object may have
// been moved by the GC

David
-----

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