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

[David Holmes]

On 4/05/2016 4:43 AM, Mikael Vidstedt wrote:
>
> Thank you very much for the review, some comments inline...
>
> On 5/3/2016 8:15 AM, Aleksey Shipilev wrote:
>> On 05/03/2016 08:09 AM, Mikael Vidstedt wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8150921
>>> Webrev (jdk):
>>> http://cr.openjdk.java.net/~mikael/webrevs/8150921/webrev.01/jdk/webrev/
>>> Webrev (hotspot/):
>>> http://cr.openjdk.java.net/~mikael/webrevs/8150921/webrev.01/hotspot/webrev/
>>>
>> Nice move overall!
>>
>> unsafe.cpp:
>>
>> *) 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!
>
>>
>> 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.
>
>> library_call.cpp:
>>
>> *) What is the point for doing this check:
>>
>> 2372   if (_gvn.type(base)->isa_ptr() != TypePtr::NULL_PTR) {
>> 2373     heap_base_oop = base;
>> 2374   }
>>
>> In old code heap_base_oop != top() only for heap ptrs, even if base is
>> NULL. In new code, if base is NULL, then heap_base_oop is top(). It
>> would seem that new code is better, because it clearly separates heap
>> vs. native accesses, and it does not seem to break anything downstream.
>> Was that the intent?
>
> Those three lines were actually donated to my by John Rose, so I'm
> hoping he or somebody else more knowledgeable in C2 can provide some
> helpful comments on the correctness and validity of that specific change.
>
>
> I also changed the normalize() methods to be private in the MemoryAccess
> class, and made the MemoryAccess class a StackObj.
>
> New webrev(s) here:
>
> 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/

src/share/vm/prims/unsafe.cpp

I think this is a bit over-generalized:

254   template <typename T>
  255   void put_mutex(T x) {
  256     GuardUnsafeAccess guard(_thread, _obj);
  257
  258     MutexLockerEx mu(UnsafeJlong_lock, 
Mutex::_no_safepoint_check_flag);
  259
  260     T* p = (T*)addr();
  261
  262     Atomic::store(normalize(x),  p);
  263   }

The UnsafeJlong lock is, as the name suggests, only intended for use 
with jlongs - we have no need of it for any other type. I don't think 
this needs to be a template method.

As noted before the lock could taken after calculating the address - to 
shorten the critical section.

If you take care to ensure no uses of MemoryAccess transitively can hit 
a safepoint then you could store the oop directly rather than the 
jobject. That would avoid multiple calls to JNIHandles::resolve (in the 
GuardUnsafeAccess and addr() function).

  405   if (VM_Version::supports_cx8()) {
  406     return MemoryAccess(thread, obj, offset).get_volatile<jlong>();

  407   } else {
  408     return MemoryAccess(thread, obj, offset).get_mutex<jlong>();

You could internalize the supports_cx8 check and the use of locking 
inside MemoryAccess.get_volatile<jlong> rather than having get_volatile 
and get_mutex. If you prefer to keep it this way then 
get_volatile_with_mutex would be a more appropriate name (and it doesn't 
need to be a template method as I said before).

Can't comment on the rest of the compiler related stuff too much, but I 
didn't see anything obviously odd looking.

Thanks,
David

>
> Incremental from webrev.01:
>
> jdk: N/A (no changes)
> hotspot:
> http://cr.openjdk.java.net/~mikael/webrevs/8150921/webrev.02.incr/hotspot/webrev/
>
>
> Cheers,
> Mikael
>
>>
>> Thanks,
>> -Aleksey
>>
>>
>