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

[Rahul Raghavan]

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

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.

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