RFR(S): 6378256: Performance problem with System.identityHashCode in client compiler
Rahul Raghavan
rahul.v.raghavan at oracle.com
Fri Feb 5 14:52:01 UTC 2016
Hi,
Please review revised patch, notes for JDK-6378256.
https://bugs.openjdk.java.net/browse/JDK-6378256
Latest webrev-03: http://cr.openjdk.java.net/~thartmann/6378256/webrev.03/
(For reference old webrev-02: http://cr.openjdk.java.net/~thartmann/6378256/webrev.02/)
Thank you Roland for the review comments.
>> Also the x86_64 and x86_32 are (mostly?) identical.
>> Do we want to create a sharedRuntime_x86.cpp, move the InlineObjectHash code
>> in its own function there to avoid duplication?
Yes agree to this point.
So for the latest revised webrev-03 patch, tried creating new sharedRuntime_x86.cpp and moving shared code related to 6378256 fix in new function.
As of now moved only the new function for 6378256 to this common file.
I believe maybe more refactoring can be done, moving other unrelated common code between x86_32 and x86_64 if any, as a separate task!
Confirmed no issues with jprt testing (-testset hotspot).
>> Can you justify the comments again?
Please refer the sample results got for manual unit tests for various targets, with and without proposed fix.
Following is small unit test, a micro benchmark test case from JDK-6182955 related to JDK-6378256.
----Test Begin-------
public class MyEnumTest {
static class Zuper {
public final int hashCode() {
return super.hashCode();
}
}
static class Identity {
public final int hashCode() {
return System.identityHashCode(this);
}
}
static long test(Object o) {
long time=System.nanoTime();
for(int i=0;i<1000000;i++)
o.hashCode();
return System.nanoTime()-time;
}
public static void main(String[] args) {
System.out.println("super "+test(new Zuper()));
System.out.println("identity "+test(new Identity()));
}
}
----Test End-----------
Note: Original C1 issue reported - Performance problem with System.identityHashCode compared to Object.hashCode, with client compiler (at least seven times slower)
---Sample unit test results------------------
[01]. x86_32 (tried linux_i586 product build)
//// Original behavior without fix
// JDK-6378256 issue reproducible mainly with -client, -XX:TieredStopAtLevel=1, 2
With -server
super 7971642
identity 6805876
With - client
super 5261808
identity 71713073
With -XX:+TieredCompilation
super 8781989
identity 8571933
With -XX:TieredStopAtLevel=1
super 6593751
identity 55797683
With -XX:TieredStopAtLevel=2
super 9619376
identity 74430911
With -XX:TieredStopAtLevel=3
super 11972884
identity 63821641
//// unit test results with latest proposed webrev-03 fix
//System.identityHashCode results now comparable similar to Object.hashCode for client)
With -server
super 8615908
identity 6604061
With - client
super 6075892
identity 6412164
With -XX:+TieredCompilation
super 7261396
identity 7328875
With -XX:TieredStopAtLevel=1
super 7207882
identity 6277642
With -XX:TieredStopAtLevel=2
super 9500328
identity 8843374
With -XX:TieredStopAtLevel=3
super 11616995
identity 9140043
--------------------------------------------
[02]. solaris_sparcv9
//// Original behavior without fix
// 64 bit target, so 6378256 issue reproducible with -XX:TieredStopAtLevel=1, 2, 3
With -server
super 14013890
identity 10942257
With - client
super 14004123
identity 11661086
With -XX:+TieredCompilation
super 16058964
identity 12464348
With -XX:TieredStopAtLevel=1
super 15114826
identity 202158637
With -XX:TieredStopAtLevel=2
super 22183247
identity 207758147
With -XX:TieredStopAtLevel=3
super 32577893
identity 256768334
//// unit test results with proposed fix
//System.identityHashCode results now comparable similar to Object.hashCode for client, after enabling the existing optimization.
With -server
super 11898144
identity 18135942
With - client
super 12272644
identity 9375612
With -XX:+TieredCompilation
super 11636948
identity 9350462
With -XX:TieredStopAtLevel=1
super 13906980
identity 10005384
With -XX:TieredStopAtLevel=2
super 22487806
identity 17352714
With -XX:TieredStopAtLevel=3
super 32788661
identity 29396398
--------------------------------------------
[03]. x86_64 (linux_x64 - product build)
//// Original behavior without fix
// In this case the C1 optimization, of inline code to check hashCode from object header, was not supported even for object.hashCode.
// So found sample test results with -XX:TieredStopAtLevel=1, 2, 3 on higher side
With -server
super 9609530
identity 12478818
With - client
super 9647637
identity 8095867
With -XX:+TieredCompilation
super 8644689
identity 9157999
With -XX:TieredStopAtLevel=1
super 30208812
identity 43761942
With -XX:TieredStopAtLevel=2
super 35847408
identity 40643575
With -XX:TieredStopAtLevel=3
super 31086749
identity 33162520
//// unit test results with latest proposed webrev-03 fix
// better results with -XX:TieredStopAtLevel=1, 2, 3
With -server
super 9430873
identity 6230543
With - client
super 9525046
identity 10886286
With -XX:+TieredCompilation
super 7275681
identity 8156844
With -XX:TieredStopAtLevel=1
super 8406237
identity 7589478
With -XX:TieredStopAtLevel=2
super 8629649
identity 7419501
With -XX:TieredStopAtLevel=3
super 12555911
identity 10658754
Please send review comments
Thank you,
Rahul
> Date: Thu, 28 Jan 2016 15:22:19 +0100 > From: Roland Westrelin <roland.westrelin at oracle.com> To: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>
>
> > 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/
>
> That looks good. Can you justify the comments again?
>
> Also the x86_64 and x86_32 are (mostly?) identical. Do we want to create a sharedRuntime_x86.cpp, move the InlineObjectHash code
> in its own function there to avoid duplication?
>
> Roland.
>
> > -----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
More information about the hotspot-compiler-dev
mailing list