RFR: 8274301: Locking with _no_safepoint_check_flag does not check NJT

[David Holmes]

On Thu, 14 Oct 2021 21:04:29 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> There are Mutexes that have a safepoint checking rank, that are locked via:
> MutexLocker ml(safepoint_lock, Mutex::_no_safepoint_check_flag);
> 
> These don't assert for the inconsistency because the thread that calls this is a NonJavaThread.
> 
> But if you lock
> MutexLocker ml(nosafepoint_lock);  // default safepoint check
> 
> These *will* assert for the safepoint checking inconsistency even for NonJavaThreads.
> 
> NonJavaThreads don't participate in the safepoint checking protocol.  And some of these safepoint checking locks are shared between java and nonjava threads. So in the existing code, there's a mix of locking with _no_safepoint_check_flag and not.
> 
> This has a wrinkle because if you have a safepoint checking lock, and call wait on it, we now have to handle the case that the caller is a NonJavaThread and should not safepoint check (just like the Mutex::lock call).
> 
> Maybe this shouldn't be fixed, and we should just document the discrepancy.
> 
> Tested with tier1-3.

This is effectively undoing some of the key changes to the API that you made in JDK-8222811! There you split wait() into two functions: wait() and wait_without_safepoint_check()  and then had MonitorLocker::wait() call the right version based on the safepoint_check flag used at construction time. And now you are making wait() implement both safepoint checking and non-safepoint checking code paths, because you want to add an extra check based on the type of the thread? Sorry but I am not seeing an improvement here - quite the opposite.

> Maybe this shouldn't be fixed, and we should just document the discrepancy.

At the moment I'd vote for documentation (if actually needed).

Cheers,
David

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

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