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

[Rahul Raghavan]

> -----Original Message-----
> From: Tobias Hartmann > Sent: Tuesday, January 26, 2016 2:40 PM > To: Rahul Raghavan; hotspot-compiler-dev at openjdk.java.net
> 
> Hi Rahul,
> 
> looks good to me (not a Reviewer). The code in sharedRuntime_x86_64.cpp is much better now!

Thank you Tobias.

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