RFR (M): 8149159: Clean up Unsafe

[Mikael Vidstedt]

Thanks for all the great feedback/questions. Updated webrev(s) here:

http://cr.openjdk.java.net/~mikael/webrevs/8149159_unsafecleanup/hotspot/webrev.01.incr/webrev/
http://cr.openjdk.java.net/~mikael/webrevs/8149159_unsafecleanup/jdk/webrev.01.incr/webrev/

Also, here are the changes after rebasing on top of hs-comp to include 
Aleksey's recent changes to Unsafe. There were some conflicts in 
unsafe.cpp where the new methods/functions were added, so I had to 
resolve that and apply the cleanup to them as well.

hotspot: 
http://cr.openjdk.java.net/~mikael/webrevs/8149159_unsafecleanup/hotspot/webrev.01/webrev/
jdk: 
http://cr.openjdk.java.net/~mikael/webrevs/8149159_unsafecleanup/jdk/webrev.01/webrev/


* Changes

Apart from resolving the conflicts in unsafe.cpp, this change include 
the following changes compared to webrev.00:

Claes - you are absolutely about right about the missing annotations, I 
have gone through and added the ForceInline annotation to all the 
s.m.Unsafe methods were it was missing. Thanks for catching that!

Chris - I chose to completely remove the UnsafeWrapper macro and its 
uses from unsafe.cpp. After all it's empty right now, and if somebody 
feels like adding it back it can simply be injected into the 
UNSAFE_ENTRY/UNSAFE_LEAF macros.


* Testing

I've run both -testset hotspot and -testset core on this, and FWIW it's 
all green.


* Comments/other

Stas - Wrt to the changes to the unsafe.cpp function names, I chose to 
rename the functions to include the suffix "0" to reflect the name 
change of the corresponding java (native) methods. While it's not 
strictly necessary it does feel just a tad clearer/more intuitive to me 
to have the names match.

John/Paul - I filed https://bugs.openjdk.java.net/browse/JDK-8150921 to 
cover the migration from single (long) addressing modes for the Unsafe 
getters/setters to the double-register (Object + long) equivalents.

Cheers,
Mikael


On 2016-02-19 12:05, Claes Redestad wrote:
> Good stuff! But quite a few delegating methods in sun.misc.Unsafe did 
> not get the @ForceInline treatment, which seems like an oversight?
>
> Thanks!
> /Claes
>
> On 2016-02-18 20:22, Mikael Vidstedt wrote:
>>
>> Please review the following change which does some relatively 
>> significant cleaning up of the Unsafe implementation.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8149159
>> Webrev (hotspot): 
>> http://cr.openjdk.java.net/~mikael/webrevs/8149159_unsafecleanup/hotspot/webrev.00/webrev/
>> Webrev (jdk): 
>> http://cr.openjdk.java.net/~mikael/webrevs/8149159_unsafecleanup/jdk/webrev.00/webrev/
>>
>> Summary:
>>
>> * To avoid code duplication sun.misc.Unsafe now delegates all work to 
>> jdk.internal.misc.Unsafe. This also means that the VM - and 
>> unsafe.cpp specifically - no longer needs to know or care about 
>> s.m.Unsafe.
>> * The s.m.Unsafe delegation methods have all been decorated with 
>> @ForceInline to minimize the risk of performance regressions, though 
>> it is highly likely that they will be inlined even without the 
>> annotations.
>> * The documentation has been updated to reflect that it is the 
>> responsibility of the user of Unsafe to make sure arguments are valid.
>> * The argument checking has, to the extent possible, been moved from 
>> unsafe.cpp up to Java to simplify the native code and allow the JIT 
>> to optimize it.
>> * Some of the argument checks have been relaxed. For example, the 
>> recently introduced U.copySwapMemory does not check for null pointers 
>> anymore. See docs for j.i.m.U.checkPointer for the complete reasoning 
>> behind this. Note that the Unsafe methods today, apart from 
>> U.copySwapMemory, do not perform the NULL related check(s).
>> * A test was added for j.i.m.U.copyMemory, based on U.copySwapMemory. 
>> Feel free to point out that I should merge them (because I should).
>>
>> Also, unsafe.cpp was cleaned up rather dramatically. Some specific 
>> highlights:
>>
>> * Unsafe_ functions are now declared static, as are the other 
>> unsafe.cpp local functions.
>> * Created unsafe.hpp and moved some functions used in other parts of 
>> the VM to that. Removed some "extern" function declarations (extern 
>> is bad, kittens die when extern is (over-)used).
>> * Remove scary looking comment about UNSAFE_LEAF not being possible 
>> to use - there's nothing special about it, it's just a JVM_LEAF.
>> * Used UNSAFE_LEAF for a few simple leaf methods
>> * Added helpful braces around UNSAFE_ENTRY/UNSAFE_END to help 
>> auto-indent
>> * Removed unused Unsafe_<...>##140 functions/macros
>> * Updated macro argument names to be consistent throughout unsafe.cpp 
>> macro definitions
>> * Replaced some checks with asserts - as per above the checks are now 
>> performed in j.i.m.Unsafe instead.
>> * Removed all the s.m.Unsafe related code
>>
>>
>> Testing:
>>
>> * jtreg: hotspot_jprt group, jdk/internal
>> * JPRT: hotspot testset
>> * Perf: JMH unsafe-bench.jar (no significant changes)
>>
>> I'm taking suggestions on additional things to test.
>>
>> Cheers,
>> Mikael
>>
>