RFR: 8242440: use separate, destroyable JavaVM instances per libgraal compiler thread

[Doug Simon]

On Sun, 17 Apr 2022 04:42:44 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Currently, libgraal runs in a single, permanent JavaVM instance loaded from libjvmcicompiler.so. This poses 2 problems:
>> 
>> 1. The memory used for libgraal is never released, even when the libgraal compiler queues are idle.
>> 2. Since all libgraal HotSpot compiler threads share the same SVM heap, libgraal compilation is effectively paused when a GC occurs (SVM currently has a stop the world collector). The more compiler threads there are, the more often this happens.
>> 
>> This PR implements the following solution to these problems:
>> 1. Create a new JavaVM instance per libgraal compiler thread (i.e. a separate SVM isolate per thread). This prevents GC in one libgraal thread from impacting another libgraal thread.
>> 2. Destroy the JavaVM instances when a libgraal compiler thread becomes idle and create a new one when subsequent compilation requests arrive.
>> 
>> Most of the changes are in JVMCI specific files. The most significant change to shared code is the addition of the `JavaThread:: _libjvmci_runtime` field. This is required as any thread (not just a `CompilerThread`) can be associated with a specific `JVMCIRuntime` object.
>
> src/hotspot/share/jvmci/jvmciRuntime.cpp line 905:
> 
>> 903:     object_handles()->release(ptrs, released);
>> 904:   }
>> 905:   _jobjects->clear();
> 
> `clear` doesn't free any memory from the growable-array, so a runtime's _jobjects will have a capacity based on the high-water usage.  I don't know if that's intentional.  I was going to suggest using `GrowableArray::clear_and_deallocate()` if not, but the initialization of the array specifies a non-zero initial capacity (100).

It's expected that libgraal isolates have roughly the same pattern in terms of jobject allocation so keeping the memory around is intentional.

> src/hotspot/share/jvmci/jvmciRuntime.cpp line 976:
> 
>> 974: JVMCIRuntime::JVMCIRuntime(JVMCIRuntime* next, int id, bool for_compile_broker) {
>> 975:   _init_state = uninitialized;
>> 976:   _shared_library_javavm = NULL;
> 
> Prefer nullptr to NULL in new code.

Thanks for the reminder. I'm tempted to now change `NULL` to `nullptr` in all `jvmci/*` files. What's the policy/recommendation on that? Don't bother/separate PR/piggy-back on this PR?

> src/hotspot/share/jvmci/jvmciRuntime.hpp line 168:
> 
>> 166:   // destroying remaining JNI handles when the JavaVM associated
>> 167:   // with this runtime is shutdown.
>> 168:   GrowableArray<jobject>* _jobjects;
> 
> I wonder if it would simplify anything to use OopHandle instead of jobject as the element type?

Interesting proposal. What would be the advantage of `OopHandle` over `jobject`? That require a bunch of `reinterpret_cast`ing between `jobject` and `OopHandle` which I'm not sure is even possible?

> src/hotspot/share/runtime/mutexLocker.hpp line 151:
> 
>> 149: 
>> 150: #if INCLUDE_JVMCI
>> 151: #define JVMCIRuntime_lock_rank safepoint
> 
> This shouldn't be a macro.  Use `const Mutex::Rank JVMCIRuntime_lock_rank = safepoint;`.

Thanks. This suggestion actually helps me clean up the code by allocating a global `JVMCIRuntime_lock` instead (associated with the first `JVMCIRuntime` created) and using it as a prototype for the lock when subsequent `JVMCIRuntime`s are created.

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

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