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

[Tobias Hartmann]

Hi Rahul,

looks good to me (not a Reviewer). The code in sharedRuntime_x86_64.cpp is much better now!

Best,
Tobias

On 25.01.2016 18:02, Rahul Raghavan wrote:
> Hello,
> 
> With reference to below email thread, please send review comments for the revised patch for JDK-6378256.
> http://cr.openjdk.java.net/~thartmann/6378256/webrev.02/
> 
> Thanks,
> Rahul
> 
>> -----Original Message-----
>> From: Tobias Hartmann > Sent: Monday, January 25, 2016 12:40 PM > To: Rahul Raghavan; hotspot-compiler-dev at openjdk.java.net
>>
>> 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