8317422: [JVMCI] concurrency issue in MethodData creation

[dougxc]

Method::build_profiling_method_data is the safe way to create MethodData for a Method - it uses an atomic to do the write.
This PR updates c2v_getFailedSpeculationAddress and c2v_reprofile to use Method::build_profiling_method_data.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8317422: [JVMCI] concurrency issue in MethodData creation (Bug - P2)

Reviewers

• Tom Rodriguez (@tkrodriguez - Reviewer) ⚠️ Review applies to fda052d4
• Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16026/head:pull/16026
$ git checkout pull/16026

Update a local copy of the PR:
$ git checkout pull/16026
$ git pull https://git.openjdk.org/jdk.git pull/16026/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16026

View PR using the GUI difftool:
$ git pr show -t 16026

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16026.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@dougxc The following labels will be automatically applied to this pull request:

• graal
• hotspot-compiler

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.

[tkrodriguez]

It's possible my diagnosis of this is wrong since set_method_data tries to handle the memory ordering by doing a release_store but maybe that's not strong enough? I see there is also release_store_fence. Anyway, I think this cleanup is a good idea even if it's not the full fix.

I think we should eliminate the public set_method_data completely. set_method_data(null) should be a new clear_method_data() and we'd need to fix ciReplay.cpp to also call build_profiling_method_data.

[dougxc]

I think we should eliminate the public set_method_data completely. set_method_data(null) should be a new clear_method_data() and we'd need to fix ciReplay.cpp to also call build_profiling_method_data.

Good idea. I made these changes.

[mlbridge]

Webrevs

• 01: Full - Incremental (618ed186)
• 00: Full (fda052d4)

[tkrodriguez]

This should be private now I think.

[dougxc]

Makes sense - done. I also inlined its definition into method.hpp.

[openjdk]

@dougxc 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:

8317422: [JVMCI] concurrency issue in MethodData creation

Reviewed-by: never, thartmann

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 46 new commits pushed to the master branch:

• 460ebcd: 8314978: Multiple server call from connection failing with expect100 in getOutputStream
• dc4bc4f: 8306819: Consider disabling the compiler's default active annotation processing
• a4e9168: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays)
• 6c6beba: 8317128: java/nio/file/Files/CopyAndMove.java failed with AccessDeniedException
• b62e774: 8317515: Unify the code of the parse*() families of methods in j.l.Integer and j.l.Long
• a64794b: 8317560: Change to Xcode 14.3.1 for building on macOS at Oracle
• b3cc0c8: 8317318: Serial: Change GenCollectedHeap to SerialHeap in whitebox
• 691db5d: 8317592: Serial: Remove Space::toContiguousSpace
• ec9ba5d: 8317660: [BACKOUT] 8269393: store/load order not preserved when handling memory pool due to weakly ordered memory architecture of aarch64
• 7162624: 8269393: store/load order not preserved when handling memory pool due to weakly ordered memory architecture of aarch64
• ... and 36 more: https://git.openjdk.org/jdk/compare/b6a97c078043862b20bd8e1d1b8ccb8699995515...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[TobiHartmann]

Looks good to me.

[dougxc]

Thanks for the reviews!

/integrate

[openjdk]

Going to push as commit a07a4a3.
Since your change was applied there have been 51 commits pushed to the master branch:

• 7ff1938: 8310727: C2: *_of() methods in PhaseIterGVN should use uint for the node index
• a57ae7e: 8317594: G1: Refactor find_empty_from_idx_reverse
• 6ddbfaa: 8317136: [AIX] Problem List runtime/jni/terminatedThread/TestTerminatedThread.java
• 0cf1a55: 8317440: Lock rank checking fails when code root set is modified with the Servicelock held after JDK-8315503
• bcc986b: 8317601: Windows build on WSL broken after JDK-8317340
• 460ebcd: 8314978: Multiple server call from connection failing with expect100 in getOutputStream
• dc4bc4f: 8306819: Consider disabling the compiler's default active annotation processing
• a4e9168: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays)
• 6c6beba: 8317128: java/nio/file/Files/CopyAndMove.java failed with AccessDeniedException
• b62e774: 8317515: Unify the code of the parse*() families of methods in j.l.Integer and j.l.Long
• ... and 41 more: https://git.openjdk.org/jdk/compare/b6a97c078043862b20bd8e1d1b8ccb8699995515...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dougxc Pushed as commit a07a4a3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.