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

[Mikael Vidstedt]

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/

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