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

Mikael Vidstedt mikael.vidstedt at oracle.com
Thu May 5 18:35:52 UTC 2016


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/

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



More information about the hotspot-runtime-dev mailing list