FW: RFR(S): 6378256: Performance problem with System.identityHashCode in client compiler

[Tobias Hartmann]

Hi Rahul,

On 22.01.2016 17:11, Rahul Raghavan wrote:
> 
>> -----Original Message-----
>> From: Tobias Hartmann > Sent: Monday, January 11, 2016 2:56 PM > To: Rahul Raghavan; hotspot-compiler-dev at openjdk.java.net
>>
>> Hi Rahul,
>>
>>> http://cr.openjdk.java.net/~thartmann/6378256/webrev.01/
>>
>> Why don't you use 'markOopDesc::hash_mask_in_place' for the 64 bit version? This should safe some instructions and you also don't
>> need the 'hash' register if you compute everything in 'result'.
> 
> Thank you for your comments Tobias.
> 
> I could not get the implementation work with the usage of 'markOopDesc::hash_mask_in_place' in x86_64 (similar to support in x86_32).
> Usage of -    __ andptr(result, markOopDesc::hash_mask_in_place);
> Results in build error - ' overflow in implicit constant conversion'
> 
> Then understood from 'sharedRuntime_sparc.cpp', 'markOop.hpp' -  that the usage of 'hash_mask_in_place' should be avoided for 64-bit because the values are too big!
> Similar comments in LibraryCallKit::inline_native_hashcode [hotspot/src/share/vm/opto/library_call.cpp] also.
> Could not find some other way to use hash_mask_in_place here for  x86_64?

You are right, I missed that.

> So depending on markOopDesc::hash_mask, markOopDesc::hash_shift value instead (similar to done in sharedRuntime_sparc)
> Added missing comment regarding above in the revised webrev.
> 
> Also yes I missed the optimized codegen.
> Tried revised patch removing usages of extra 'hash', 'mask' registers and computed all in 'result' itself.
> 
> [sharedRuntime_x86_64.cpp]
>      ....................
> +    Register obj_reg = j_rarg0;
> +    Register result = rax;
>      ........
> +    // get hash
> +    // Read the header and build a mask to get its hash field.
> +    // Depend on hash_mask being at most 32 bits and avoid the use of hash_mask_in_place
> +    // because it could be larger than 32 bits in a 64-bit vm. See markOop.hpp.
> +    __ shrptr(result, markOopDesc::hash_shift);
> +    __ andptr(result, markOopDesc::hash_mask);
> +    // test if hashCode exists
> +    __ jcc  (Assembler::zero, slowCase);
> +    __ ret(0);
> +    __ bind (slowCase);
>       ........
> 
> Confirmed no issues with jprt testing (-testset hotspot) and expected results for unit tests.
> 
> Please send your comments. I can submit revised webrev if all okay.

Looks good. Please send a new webrev.

Best,
Tobias

> 
>>
>> Best,
>> Tobias
>>
>>
>> On 08.01.2016 18:13, Rahul Raghavan wrote:
>>> Hello,
>>>
>>> Please review the following revised patch for JDK-6378256 -
>>> http://cr.openjdk.java.net/~thartmann/6378256/webrev.01/
>>>
>>> This revised webrev got following changes -
>>>
>>>  1) A minor, better optimized code with return 0 at initial stage (instead of continuing to 'slowCase' path), for special/rare null
>> reference input!
>>>    (as per documentation, test results confirmed it is safe to 'return 0' for null reference input, for System.identityHashCode)
>>>
>>>  2) Added similar Object.hashCode, System.identityHashCode optimization support in sharedRuntime_x86_64.cpp.
>>>
>>> Confirmed no issues with jprt testing (-testset hotspot) and expected results for unit tests.
>>>
>>> Thanks,
>>> Rahul
>>>
>>>
>>>> -----Original Message-----
>>>> From: Roland Westrelin > Sent: Wednesday, December 09, 2015 8:03 PM > To: Rahul Raghavan> Cc: hotspot-compiler-
>> dev at openjdk.java.net
>>>>
>>>>> webrev: http://cr.openjdk.java.net/~thartmann/6378256/webrev.00/ .
>>>>
>>>> Justifying the comment lines 2019-2022 in sharedRuntime_sparc.cpp (lines 1743-1746 in sharedRuntime_x86_32.cpp) again would
>> be
>>>> nice.
>>>> Shouldn't we use this as an opportunity to add the same optimization to sharedRuntime_x86_64.cpp?
>>>>
>>>> Roland.
>>>
>>>
>>>> -----Original Message-----
>>>> From: Rahul Raghavan > Sent: Wednesday, December 09, 2015 2:43 PM > To: hotspot-compiler-dev at openjdk.java.net
>>>>
>>>> Hello,
>>>>
>>>> Please review the following patch for JDK-6378256.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~thartmann/6378256/webrev.00/ .
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-6378256  .
>>>> Performance problem with System.identityHashCode, compared to Object.hashCode, with client compiler (at least seven times
>>>> slower).
>>>> Issue reproducible for x86_32, SPARC (with -client / -XX:TieredStopAtLevel=1 , 2, 3 options).
>>>>
>>>> sample unit test:
>>>>    public class Jdk6378256Test
>>>>    {
>>>>       public static void main(String[] args)
>>>>       {
>>>>          Object obj = new Object();
>>>>          long time = System.nanoTime();
>>>>          for(int i = 0 ; i < 1000000 ; i++)
>>>>             System.identityHashCode(obj);  //compare to obj.hashCode();
>>>>          System.out.println ("Result = " + (System.nanoTime() - time));
>>>>       }
>>>>    }
>>>>
>>>> Fix: Enabled the C1 optimization which was done only for Object.hashCode, now for System.identityHashCode() also.
>>>> (looks in the header for the hashCode before calling into the VM).
>>>> Unlike for Object.hashCode, System.identityHashCode is static method and gets object as argument instead of the receiver.
>>>> So also added required additional null check for System.identityHashCode case.
>>>>
>>>> Testing:
>>>>    - successful JPRT run (-testset hotspot).
>>>>    - JTREG testing (hotspot/test, jdk/test - java/util, java/io, java/lang/System).
>>>>        (with -client / -XX:TieredStopAtLevel=1 etc. options).
>>>>    - Added 'noreg-perf' label for this performance bug.
>>>>       Manual testing done and confirmed expected performance values for unit tests with fix.
>>>>
>>>> Thanks,
>>>> Rahul