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

[David Holmes]

Hi Mikael,

On 6/05/2016 4:35 AM, Mikael Vidstedt wrote:
>
> Updated webrevs here:
>
> jdk:
> http://cr.openjdk.java.net/~mikael/webrevs/8150921/webrev.03/jdk/webrev/
> hotspot:
> http://cr.openjdk.java.net/~mikael/webrevs/8150921/webrev.03/hotspot/webrev/
>
>
> Incremental from webrev.02:
>
> jdk:
> http://cr.openjdk.java.net/~mikael/webrevs/8150921/webrev.03.incr/jdk/webrev/
>
> hotspot:
> http://cr.openjdk.java.net/~mikael/webrevs/8150921/webrev.03.incr/hotspot/webrev/

Sorry I'm going to have to insist on a minor renaming as we are not 
getting or putting mutexes. How about get_jlong_locked/put_jlong_locked ?

No need to see another webrev.

Thanks,
David

>
> Changes from webrev.02:
>
> - javadoc updates for getAddress/putAddress in line with Paul's feedback
> (move the more descriptive comment to the double-register methods, with
> an @see on the single-reg guys)
> - updates to make MemoryAccess::get_mutex jlong specific
>
> Some additional comments inline..
>
> On 5/3/2016 10:07 PM, David Holmes wrote:
>> 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.
>
> Good point. I guess the method could be made to take the mutex as an
> argument, or internally decide which mutex to use depending on the type,
> but given that really only is used for jlong right now I made the
> function jlong specific (without the template argument).
>
>> 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).
> Since the resolve call is really just dereferencing a pointer and the
> data should already be in the L1 cache I'm not sure this is worth
> optimizing a whole lot more, but if we really think it will actually
> make a difference we can definitely store the raw oop instead. I chose
> to leave it the way it is for now.
>
>>
>>  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).
>
> That's good, I like it! In the interest of drawing a line though I'm
> going to choose to leave that for a future enhancement.
>
> Cheers,
> Mikael
>
>>
>> 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
>>>>
>>>>
>>>
>