RFR: 8221360: Eliminate Shared_DirtyCardQ_lock

[Thomas Schatzl]

On Sat, 14 Aug 2021 23:57:58 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> Please review this change to the handling of a very rare race condition in
> concurrent refinement.  There is a situation where we many be unable to
> process a card and need to re-dirty and re-enqueue it for later processing.
> 
> The old code handled this using a shared dirty card queue, which various
> other places needed to know about as well.  This was the user of the
> Shared_DirtyCardQ_lock.  It was the only remaining Mutex whose rank is based
> on Mutex::access.
> 
> An option for eliminating that lock was to use a lock-free queue.  But
> rather than take on that complexity, we instead brute-force the problem.
> The new implementation simply allocates and enqueues an entire buffer for
> the one card that needs to be reprocessed.  Those operations are already
> lock-free.  The extra cost doesn't really matter, since this situation
> almost never arises.
> 
> This is now done inline in refine_card_concurrently.  It's not worth the
> overhead of a class for what is now just a few lines of code needed in
> exactly one place.  Places that knew about the shared queue in order to
> flush/reset it at certain points no longer have such responsibilities.
> 
> This change also renders JDK-8214997 moot, since there aren't any 'access'
> locks.
> https://bugs.openjdk.java.net/browse/JDK-8214997
> Crash holding 'access' lock can deadlock in JVMCI compiler thread
> 
> This change also allows the removal of Mutex::access.  That's being left to
> a new enhancement, since I think it also allows removing Mutex::tty,
> reverting back to using Mutex::event for the tty_lock.  That is, essentially
> undo JDK-8214315.
> https://bugs.openjdk.java.net/browse/JDK-8272480
> 
> Testing:
> mach5 tier1-5

Another reason probably why these cards are put into the shared DCQS was that that card will be processed at a time when that situation can't re-occur. So this change might cause that card (buffer) to be re-enqueued and reprocessed many times while waiting for that object to get fully visible.
One option to avoid this would be to temporarily raise the refinement threshold to avoid reprocessing, but it is very hard to know when it can be lowered (apart from complications actually doing that).
Another would be to use a non-dcqs lock-free container (e.g. a segmented array) to store all these entries (there may be some implementations around), but that's probably not worth it either, and somewhat ugly to transfer to actual dcqs later for processing.
I assume and I agree that this is a very rare occurrence, so we probably get away with this solution.

src/hotspot/share/gc/g1/g1RemSet.cpp line 1699:

> 1697:   // enqueuing an entire buffer for just this card.
> 1698:   *card_ptr = G1CardTable::dirty_card_val();
> 1699:   G1DirtyCardQueueSet& dcqs = G1BarrierSet::dirty_card_queue_set();

This is just a suggestion: maybe put this code to put and enqueue that card into an extra buffer into an extra method. It does not seem to fit just here.

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

Marked as reviewed by tschatzl (Reviewer).

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