-
Notifications
You must be signed in to change notification settings - Fork 126
8364191: [lworld] Accesses to atomic flat fields prevent scalar replacement #1518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back qamai! A progress list of the required criteria for merging this PR into |
|
@merykitty This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
|
Thanks for working on this @merykitty! I haven't reviewed this in yet but noticed a build failure: |
|
@TobiHartmann Ah dummy mistake, I missed the |
|
Hi @merykitty Sorry for the delay in reviewing this, I was busy with the the array metadata refactoring and hunting down nasty AArch64 crashes (JDK-8364579 / JDK-8365996). This is great work! I see that you converted the PR back to draft, is it still ready to review? |
|
@TobiHartmann Hi Tobias, I am having difficulties understanding how our current flat accesses deal with GC barriers. IIUC, every time an access is made to a memory region that may contain an oop, we need a GC barrier (which itself may be a nop). However, looking at our current implementation, I only see that being handled for G1GC with |
|
@merykitty Hi Quan-Anh, sorry for the delay, I missed this message.
G1 is handled specially because of late barrier expansion: valhalla/src/hotspot/share/opto/inlinetypenode.cpp Lines 660 to 693 in c1774b0
For the other GCs, we emit the barriers already during parsing (the information is propagated via valhalla/src/hotspot/share/gc/shared/c1/modRefBarrierSetC1.cpp Lines 40 to 53 in c1774b0
and valhalla/src/hotspot/share/gc/shared/c2/modRefBarrierSetC2.cpp Lines 59 to 70 in c1774b0
I added this with #1318 and as mentioned in the PR, this is a bit of a hacky first implementation. We should refactor / improve this later (tracked by JDK-8350865). Does that help? If you get a chance, could you merge this PR with latest |
|
@TobiHartmann Thanks a lot for your help, I am working on this. However, I think that would mean Shenandoah is broken as I see no similar handling in |
|
@merykitty Yes, I think that's true. I only implemented Oracle-supported builds/configurations for now. Thank you! |
|
@TobiHartmann Please review this PR, thanks a lot. |
|
I did some quick testing and I'm seeing this failure: The |
|
@TobiHartmann Thanks, the issue seems to be due to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, Quan-Anh! This looks good to me. Testing is still running but looks good so far (we had some breakage in our CI). I'll get back to you once it passed.
| // element. The order of the Proj node is the same as that of _vk->_nonstatic_fields, and the null | ||
| // marker if existing will be the last Proj output. This node acts as if the load happens | ||
| // atomically and will be expanded to loading the whole payload and extracting the flattened fields | ||
| // from the loaded payload. In special cases, such as when the object from which this load read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // from the loaded payload. In special cases, such as when the object from which this load read | |
| // from the loaded payload. In special cases, such as when the object from which this load reads |
| // from the loaded payload. In special cases, such as when the object from which this load read | ||
| // does not escape, this node can be expanded to multiple loads from each flattened field. | ||
| // This node allows us to replace its results with the value from a matching store because the | ||
| // payload value cannot be directly propagated if it contains oops. This effect, in turns, allows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // payload value cannot be directly propagated if it contains oops. This effect, in turns, allows | |
| // payload value cannot be directly propagated if it contains oops. This effect, in turn, allows |
| }; | ||
|
|
||
| // Store an InlineTypeNode to a flat element, the store acts as if it is atomic. Similar to | ||
| // LoadFlatNode, this node is expanded to storing a payload creating from the field values of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // LoadFlatNode, this node is expanded to storing a payload creating from the field values of the | |
| // LoadFlatNode, this node is expanded to storing a payload created from the field values of the |
src/hotspot/share/opto/escape.cpp
Outdated
| } | ||
| } | ||
|
|
||
| // Atomic flat accesses on non-escape objects can be optimized to non-atomic accesses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Atomic flat accesses on non-escape objects can be optimized to non-atomic accesses | |
| // Atomic flat accesses on non-escaping objects can be optimized to non-atomic accesses |
|
All testing passed! I just see a few timeouts with When working on #1672, I also hit and fixed the issue you fixed in |
|
Thanks very much, I have resolved the merge conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, still looks good!
|
/integrate |
|
Going to push as commit 2f89dea. |
|
@merykitty Pushed as commit 2f89dea. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
Flat accesses prevent scalar replacement because they are mismatched accesses. It is also generally not possible to look through them, because the payload may contain an oop in the form of raw bits. As a result, this PR adds
LoadFlatNodeandStoreFlatNode, which act as high-level abstractions for atomic accesses on flat fields. When it is determined that there is no racing access on the flat field (e.g. because the holder object does not escape), these flat access nodes can be expanded into multiple accesses to each flattened fields, otherwise, they will be expanded into a sequence of inferring a payload and accessing memory with that payload.I also fix an issue with deoptimization reallocation where we miss assigning the null marker of elements in a nullable flat array.
Please take a look and leave your reviews, thanks a lot.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1518/head:pull/1518$ git checkout pull/1518Update a local copy of the PR:
$ git checkout pull/1518$ git pull https://git.openjdk.org/valhalla.git pull/1518/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1518View PR using the GUI difftool:
$ git pr show -t 1518Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1518.diff
Using Webrev
Link to Webrev Comment