RFR: 8263107: PSPromotionManager::copy_and_push_safe_barrier needs acquire memory barrier

[Albert Mingkun Yang]

On Sat, 5 Jun 2021 03:30:42 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> 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.

src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp line 308:

> 306:     }
> 307: 
> 308:     // don't update this before the unallocation!

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 of`new_obj` is used for deallocation. I think it's best to remove this comment; it doesn't really say anything interesting.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4371