8263107: PSPromotionManager::copy_and_push_safe_barrier needs acquire memory barrier

[kimbarrett]

Please review this change to PSPromotionManager::copy_to_survivor_space
(ParallelGC) to remove some redundant work, and to add some missing memory
barriers.

There are two callers of copy_to_survivor_space, both of which wrap that
call with the idiom

if obj->is_forwarded() then
new_obj = obj->forwardee()
else
new_obj = copy_to_survivor_space(obj)
endif

There are problems with this.

(1) The first thing copy_to_survivor_space does is check whether the object
is already forwarded, and if so then return obj->forwardee_acquire(). The
idiom used by the callers is a redundant check, and the redundancy can't be
optimized away. It is also missing the acquire barrier that was added by
JDK-8154736 after long discussion.

(2) It turns out the forwardee_acquire() from JDK-8154736 isn't sufficient
after all. The "if is_forwarded() then use forwardee()" idiom is hiding
under the abstractions that we're doing two relaxed atomic loads of the mark
word, and there is nothing here to prevent the second from reading a value
older than that read by the first, with bad consequences. This possibility
came up in the discussion of JDK-8154736, but seems to have been either lost
or discounted. If you think loads from the same location can't do that, see
JDK-8229169 for a counter example.

Part of this change involves removing the conditionalization of the calls to
copy_to_survivor_space; just call it directly. However, it turns out that
some compilers don't inline copy_to_survivor_space because of its size. So
we refactored it into two functions, one doing the already marked check and
then calling the other to do most of the work. This is enough for the check
to be inlined into callers, so we've effectively removed the redundant inner
check. Note: This part of the change introduces a large block of whitespace
differences due to removal of an if-else and outdenting the body; I recommend
using a view that suppresses those when reviewing.

The second part of the change involves adding or moving some acquire barriers.

(a) For the initial check whether the object is already marked, if it is
then add an acquire fence before returning the forwardee. We could instead
use a load-acquire to obtain the mark word, but that would be an unneeded
acquire barrier on the much more common unmarked case. Also removed
forwardee_acquire(), which is no longer used.

(b) If the cmpxchg race is lost, add an acquire fence before fetching and
returning the forwardee. The failed release-cmpxchg effectively behaves
like a relaxed-load, which must preceed the forwardee access and any reads
from it.

I've also changed to only log copying when actually copied, not when already
copied and forwarded. Also changed a guarantee to an assert.

I looked at all uses of forwardee() in light of problem (2), and did not
find any additional problems. (That doesn't mean there aren't any, just
that I didn't spot any. This is low-level atomics, after all.)

Testing:
mach5 tier1-3,5,7 (tier3,5,7 are where a lot of ParallelGC testing is done).
Performance testing showed no significant change.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8263107: PSPromotionManager::copy_and_push_safe_barrier needs acquire memory barrier

Reviewers

• Ivan Walulya (@walulyai - Committer) ⚠️ Review applies to caef6a5
• Thomas Schatzl (@tschatzl - Reviewer) ⚠️ Review applies to 9ded099
• Martin Doerr (@TheRealMDoerr - Reviewer) ⚠️ Review applies to 9ded099

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4371/head:pull/4371
$ git checkout pull/4371

Update a local copy of the PR:
$ git checkout pull/4371
$ git pull https://git.openjdk.java.net/jdk pull/4371/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4371

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4371.diff

[bridgekeeper]

👋 Welcome back kbarrett! 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]

@kimbarrett The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (8bf3b272)
• 01: Full - Incremental (9ded099a)
• 00: Full (caef6a51)

[walulyai]

lgtm!

[tschatzl]

Lgtm.

[tschatzl]

Maybe add a comment here that copy_unmarked_to_survivor_space also guarantees this for all paths (i.e. both successful as well as failing CAS). Maybe this is not the correct place here, maybe in a comment for copy_unmarked_to_survivor_space; or something like "copy_to_survivor_space acts as an acquire" or something, so that the returned object can be used "safely" - idk.

[openjdk]

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

8263107: PSPromotionManager::copy_and_push_safe_barrier needs acquire memory barrier

Reviewed-by: iwalulya, tschatzl, mdoerr

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

• d4377af: 8263303: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint
• dd1cbad: 8268285: vmTestbase/nsk/jvmti/GetThreadState/thrstat002 failed with "Wrong thread "thr1" (...) state after SuspendThread"

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

[TheRealMDoerr]

I have trouble understanding (2). I have no idea how it can happen that reading the same volatile memory location a second time can retrieve an older value. Regarding JDK-8229169, age() and age_top() read different sizes. The ordering issue was related to different Bytes which weren't read before AFAICS.

[albertnetymk]

This comment gives me the impression that there is some racy going on here, and deallocation and update to new_obj must be ordered this way. However, the actual reason is that the old value ofnew_obj is used for deallocation. I think it's best to remove this comment; it doesn't really say anything interesting.

[kimbarrett]

That comment went away as part of latest change to avoid reloading the forwardee.

[kimbarrett]

I have trouble understanding (2). I have no idea how it can happen that reading the same volatile memory location a second time can retrieve an older value. Regarding JDK-8229169, age() and age_top() read different sizes. The ordering issue was related to different Bytes which weren't read before AFAICS.

Dredging through the Standard, the C++ memory model does require read-read
coherence, i.e. if there are sequential reads of an "atomic object" then the
second cannot obtain an older value than the first. (C++14 1.9/13-14,
1.10/14, 1.10/18). Of course, we're outside the Standard, since we aren't
using std::atomic<>. But it seems likely to be okay to assume read-read
coherence for our Atomic accesses.

It's unclear to me what the Standard says about a case like JDK-8229169. The
code is reading a 32bit value, then later reading a 64bit value from the
same location. That can't be described within the Standard, since "atomic
object" is defined in terms of std::atomic<>. C++20 adds std::atomic_ref<>
which can temporarily make an existing object atomic, but that doesn't cover
something like this either. So maybe the reordering behavior leading to
that bug report is allowed? Of course, that code is also outside the
Standard since it is writing to one union member and reading from another.

Be that as it may, I've found a way to entirely bypass the whole question.
Rather than using cas_forward_to and later rereading the forwardee, instead
use forward_to_atomic and use the returned forwardee.

Mostly done re-running tests.

[TheRealMDoerr]

Thanks a lot for checking! That makes sense.

[tschatzl]

Still good.

[TheRealMDoerr]

Looks good. + Nice cleanup!

[kimbarrett]

Thanks @TheRealMDoerr , @tschatzl , @walulyai for reviews.

[kimbarrett]

/integrate

[openjdk]

@kimbarrett Since your change was applied there have been 2 commits pushed to the master branch:

• d4377af: 8263303: C2 compilation fails with assert(found_sfpt) failed: no node in loop that's not input to safepoint
• dd1cbad: 8268285: vmTestbase/nsk/jvmti/GetThreadState/thrstat002 failed with "Wrong thread "thr1" (...) state after SuspendThread"

Your commit was automatically rebased without conflicts.

Pushed as commit 5a66628.

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