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

Fri May 6 21:40:13 UTC 2016

On 5/5/2016 2:56 PM, David Holmes wrote:
> 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.

Ah, yes, fully agree. Will change before I push.

David/Paul/Aleksey/John - thanks for reviews and feedback!

Cheers,
Mikael

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