RFR: 8284811: Shrink critical section of JNICritical_lock [v2]

[Kim Barrett]

On Wed, 13 Apr 2022 08:42:57 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> This refactoring around gc-locker consists of two commits:
>> 
>> 1. Remove `_debug_jni_lock_count` because `verify_critical_count` already verifies #threads in critical regions. `_debug_jni_lock_count` doesn't introduce anything new.
>> 
>> 2. Shrink the critical section associated with `JNICritical_lock` to accesses to `_needs_gc` and nothing else.
>> 
>> Test: tier1-6
>
> Albert Mingkun Yang has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
> 
>   finer cr

Changes requested by kbarrett (Reviewer).

src/hotspot/share/gc/shared/gcLocker.inline.hpp line 35:

> 33:   if (!thread->in_critical() && needs_gc()) {
> 34:     stall_until_no_needs_gc(thread);
> 35:     assert(!needs_gc(), "postcondition");

I think this assertion is incorrect. Between the end of the stall and the
assertion, another thread could enter_critical and another GC could be
suppressed, setting _needs_gc, and causing the assert to fail.

And now I'm seriously questioning this whole change.

The point of jni_lock is to do the first (non-recursive) enter "carefully",
under the JNICritical_lock to eliminate race conditions, and this change has
lost that, I think incorrectly. Similarly for jni_unlock on the last exit.

Eliminating _debug_jni_lock_count might be okay (I've not checked it
carefully), but I'm not convinced of the rest.

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

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