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

[Albert Mingkun Yang]

On Thu, 14 Apr 2022 07:45:39 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

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

The `jni_lock` and `jni_unlock` naming and the matching increment/decrement gives one the impression that one is undoing the effect of the other. However, I believe that impression is incorrect. The correct value of `_jni_lock_count` is set by `set_jni_lock_count` inside a safepoint. IOW, the increment inside `jni_lock` will always be overwritten.

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

A thread running between the end of the stall and the end of the JNI API (e.g. `GetStringCritical`) will prevent safepoint from being reached and `_needs_gc` is set inside a safepoint, so this assertion is correct I think.

PS: The doc in `SafepointSynchronize::arm_safepoint` helped me greatly to understand when a safepoint is possible.

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

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