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

I don't understand how this change is supposed to work, and think it doesn't.
It would help to have some description of how the new code is (supposed to)
work, and how that's an improvement over the old code.

src/hotspot/share/gc/shared/gcLocker.cpp line 109:

> 107: }
> 108: 
> 109: void GCLocker::stall_until_no_needs_gc(JavaThread* thread) {

The increment of _jni_lock_count seems to have gotten lost.  I don't see how
this change can work without that.

src/hotspot/share/gc/shared/gcLocker.cpp line 129:

> 127:   assert(!thread->in_critical(), "shouldn't currently be in a critical region");
> 128:   assert(needs_gc(), "precondition");
> 129:   jint old_value = Atomic::fetch_and_add(&_jni_lock_count, -1);

Even assuming it's okay to modify _jni_lock_count outside the lock (which I'm not
at all sure about), I'm not convinced it's an improvement to do so.

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

Changes requested by kbarrett (Reviewer).

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