8252185: [lworld] Improve performance of identityHashCode for value objects - #32144
8252185: [lworld] Improve performance of identityHashCode for value objects#32144marc-chevalier wants to merge 13 commits into
Conversation
|
👋 Welcome back mchevalier! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@marc-chevalier The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
Webrevs
|
TobiHartmann
left a comment
There was a problem hiding this comment.
Great work Marc! I did a first pass and added a few comments. Will take another look tomorrow with a fresh brain 😄
| public void run2() { | ||
| int SIZE = 100; | ||
| for (int i = 0; i < SIZE; ++i) { | ||
| Asserts.assertEQ(h_byte(new Byte((byte)i)), h(new Byte((byte)i))); |
There was a problem hiding this comment.
Negative values are missing here.
There was a problem hiding this comment.
Added, but I now get a test failure (line 484) during warmup. PrintCompilation confirms nothing relevant is compiled yet... I'll look at that, but it's very puzzling.
| IfNode* iff_is_long_payload = create_and_map_if(control(), is_long_payload_bol, PROB_FAIR, COUNT_UNKNOWN); | ||
|
|
||
| // Case 2. one segment, less than 8-byte long | ||
| Node* result_int = AddI(MulI(intcon(31), result_empty), ConvL2I(obj_extracted)); |
There was a problem hiding this comment.
I'm not convinced that this is correct for signed byte and short. Isn't ConvL2I omitting the sign bit?
There was a problem hiding this comment.
Yes, something is off, but I can't make it fail. Not sure why not.
This PR replaces the Valhalla draft PR openjdk/valhalla#2642, which was not integrated due to the code freeze immediately before the Valhalla mainline integration.
It is conceptually very similar to acmp: it has two parts.
Static Expansion
If the operand of
identityHashCodeis known at compile-time, we can basically inline the implementation ofValueObjectMethods.valueObjectHashCode. There are a few points worth noting.int, the likely outcome is that it would get both at once with a single call togetLong. This is fine, we can do that, but we need to mark the access as mismatch and unsafe (but aligned, since the acmp maps are smartly done). But in the case we have a class made of a singleint, it would be unfortunate to mark thegetIntaccess as mismatch since it prevents some optimizations. So if we are getting exactly a field, we detect it, and we mark the load as non-mismatch (match?).Fast Path
This supports only objects with a simple shape: one segment of data, being 1, 2, 4 or 8-byte long, which is enough to cover many migrated classes (but not dates for instance). This feature is morally very similar to the acmp fast path. It is controlled by the diagnostic flag
UseHashcodeFastPath. Alike acmp, we need to sabotage the fast path in case we can do a static expansion later.I can only suggest you take a look at
inlineKlass.hppon how that works.Benchmarking
Microbenchmarking is rather unsurprising:
nullwas and is still fast (1-2ns)Full results of valhalla.hash.FastPath
Before:
After:
On real benchmark, improvements are noticeable only on hash-intensive cases.
Future Work
We can also add profiling and speculate on the type of the argument, as we do for acmp. That is left as an exercise for the reader.
The current implementation of both the static expansion and the fast path are not caching the result in the header. There are a few reasons for that:
JVM_IHashCode.Thanks,
Marc
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/32144/head:pull/32144$ git checkout pull/32144Update a local copy of the PR:
$ git checkout pull/32144$ git pull https://git.openjdk.org/jdk.git pull/32144/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 32144View PR using the GUI difftool:
$ git pr show -t 32144Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/32144.diff
Using Webrev
Link to Webrev Comment