Add option to limit identity hash code to positive values #3198
Conversation
Add command line option "-XX:[+|-]PositiveIdentityHash" to enable/disable forcing identity hash code to be a positive value. Fixes: eclipse-openj9#3062 Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>
|
jenkins test sanity zlinux jdk8 |
There was a problem hiding this comment.
Change looks good. @vijaysun-omr What perf testing do you think this needs?
|
Thanks @DanHeidinga and @gacholio for this change. I think a small unit test that computes indentityHashCode many times should tell us the relative cost of adding the option check on this code path. The baseline would be a build without this change. Maybe Gac can show the data from that test. I can also get Liberty tried with this change just to make sure it does not regress. I believe some code in a Lucene test we had exercised identity hash codes but I cannot be sure but I'll ask the team members I was working with at that time if we still have the test. |
|
I'd like some indication of how much of slowdown we'll take enabling the option to include in the doc. I expect it will be negligible in practice since anyone using a hash code is going to do something to make it positive before it could be used as a bucket index, etc. |
|
I asked and we don't have the Lucene testing I was thinking of anymore. So I will just get this evaluated on Liberty. |
|
-Xint microbench: Before: 100000000 iterations took 36859 ms After: 100000000 iterations took 36080 ms No measureble difference. |
|
-Xjit results (10x the iteration count of -Xint): Before: 1000000000 iterations took 9923 ms After: 1000000000 iterations took 10815 ms Seems consistently slower. |
|
I assume this is comparing a build without the option vs a build with the option (but the option is not specified on the command line) ? |
|
@vijaysun-omr The JIT seems to recognize and replaced identityHashCode, but it looks like that's guarded by an option that is not set, and the replacement code is unimplmeneted. Can you confirm? I can't explain why the JIT would be slower (the JITs were not identical in the builds, but they were no more than a day different). |
|
Yes, the "before" is a build from today and the "after" is a build from yesterday with this PR added. The new option was not enabled. |
|
I've never seen identityHashCode take more than a couple of percent in any profile really; it's much lower than that in the profiles of benchmarks we work with usually. So I am thinking this is acceptable since the overhead of the extra check you added would be < 0.2% in those benchmarks. |
|
There is a Fast JNI implementation in use by the JIT (and that will get the change from this PR). |
|
Is it possible to swap the JIT module between these 2 builds and see if we can get a pair of runs with literally the same JIT ? |
|
@vijaysun-omr For the Liberty bench, it would be good to know the cost of enabling the new option, not just the cost of the new code. |
|
Copying the "before" JIT into the "after" VM: 1000000000 iterations took 10844 ms yields essentially identical results. |
|
@vijaysun-omr @gacholio I'm OK with merging this now so it makes the 0.11.0 release and we can update with the Liberty results when they're available. Any objections? |
|
I guess I can just say "unknowable performance impact" in the doc. |
|
Given that I don't expect any measurable regression in Liberty I have no issue merging this now without waiting for the Liberty result. |
|
@gacholio yes we don't do anything special for the identityHashCode computation in the JIT as far as I know. |
|
Doc issue: eclipse-openj9/openj9-docs#109 |
Add command line option "-XX:[+|-]PositiveIdentityHash" to
enable/disable forcing identity hash code to be a positive value.
Doc issue eclipse-openj9/openj9-docs#109
Fixes: #3062
Signed-off-by: Graham Chapman graham_chapman@ca.ibm.com