8378862: [lworld] flatArrayKlass::copy_array is missing a memory barrier when copying from flat array to non-flat array

[fparain]

Small patch fixing a missing memory barrier in FlatArrayKlass::copy_array.
The unit test covers more scenario where a flat value has to be buffered before to be written to another heap variable. So far, these tests haven't shown any other issue (they were run repeatedly 50 times using Mach5).

However, the unit test is quite heavy, testing 5 different scenarios, and running each for 3 minutes, using all detected cores of the machine each time. This might qualify this unit test as a "resourcehog" test, it then should be moved to test/hotspot/jtreg/resourcehogs/

Fred

---

Progress

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

Issue

• JDK-8378862: [lworld] flatArrayKlass::copy_array is missing a memory barrier when copying from flat array to non-flat array (Bug - P3)

Reviewers

• Axel Boldt-Christmas (@xmas92 - Committer) ⚠️ Review applies to ed6137ba
• Ivan Walulya (@walulyai - no project role) ⚠️ Review applies to ed6137ba
• Stefan Karlsson (@stefank - Committer)

Contributors

• Axel Boldt-Christmas <aboldtch@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2183

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2183.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

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

8378862: [lworld] flatArrayKlass::copy_array is missing a memory barrier when copying from flat array to non-flat array

Co-authored-by: Axel Boldt-Christmas <aboldtch@openjdk.org>
Reviewed-by: aboldtch, iwalulya, stefank

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 1 new commit pushed to the lworld branch:

• 432870d: 8379184: [lworld] Cleanup fieldLayoutBuilder.cpp to remove get_size_and_alignment

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld branch.
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 lworld branch, type /integrate in a new comment.

[mlbridge]

Webrevs

• 04: Full - Incremental (e29b6580)
• 03: Full - Incremental (46e44c0a)
• 02: Full - Incremental (4ee0581f)
• 01: Full - Incremental (697ac998)
• 00: Full (ed6137ba)

[fparain]

/contributor add @xmas92

[openjdk]

@fparain
Contributor Axel Boldt-Christmas <aboldtch@openjdk.org> successfully added.

[xmas92]

lgtm. (Nice tests)

I think that eventually we would like to get these semantics into jcstress, it has been a good tool for finding some breakage in the JMM in the past.

[walulyai]

LGTM!

[stefank]

Note that this requirement makes the flatArrayOopDesc::obj_at much more dangerous to use than the old objArrayOopDesc::obj_at. Every place where we use this function and then make a publishing store needs to remember to perform this barrier. I wonder if we shouldn't find another, more conspicuous name that makes the users / readers aware that this is can be dangerous to use. Alternatively, push the barrier into the obj_at function?

[fparain]

Note that this requirement makes the flatArrayOopDesc::obj_at much more dangerous to use than the old objArrayOopDesc::obj_at. Every place where we use this function and then make a publishing store needs to remember to perform this barrier. I wonder if we shouldn't find another, more conspicuous name that makes the users / readers aware that this is can be dangerous to use. Alternatively, push the barrier into the obj_at function?

It's a fair point. The safest approach would be to add the barrier in the buffering code itself, so no code getting a freshly buffered value, knowingly or not, would have to worry about this memory issue. This would mean adding the barrier in FlatValuePayload::read(TRAPS).

I'll move the barrier and update the test accordingly.

[stefank]

To me this looks like a safer approach. Thanks. I've not reviewed the test.

[fparain]

Thanks @xmas92 @stefank @walulyai @lmesnik for the reviews.
Thanks @xmas92 for the initial version of the test.
/integrate

[openjdk]

Going to push as commit 12012cc.
Since your change was applied there has been 1 commit pushed to the lworld branch:

• 432870d: 8379184: [lworld] Cleanup fieldLayoutBuilder.cpp to remove get_size_and_alignment

Your commit was automatically rebased without conflicts.

[openjdk]

@fparain Pushed as commit 12012cc.

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