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

[Rahul Raghavan]

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