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

[Kim Barrett]

On Fri, 15 Apr 2022 13:29:43 GMT, Doug Simon <dnsimon 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.

I've only carefully looked at the runtime changes; only skimming the jvmci and compiler parts.

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

src/hotspot/share/jvmci/jvmciRuntime.cpp line 975:

> 973: 
> 974: JVMCIRuntime::JVMCIRuntime(JVMCIRuntime* next, int id, bool for_compile_broker) {
> 975:   _init_state = uninitialized;

Why is this constructor using assignments rather than a ctor-initializer?  There's lots of old code that doesn't, and the Style Guide doesn't require it (yet - https://bugs.openjdk.java.net/browse/JDK-8252586), but that's the trend (and generally considered best practice).

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.

src/hotspot/share/jvmci/jvmciRuntime.cpp line 1185:

> 1183: JNIEnv* JVMCIRuntime::init_shared_library_javavm() {
> 1184:   MutexLocker locker(_lock);
> 1185:   JavaVM* javaVM = (JavaVM*) _shared_library_javavm;

Pointless cast.

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 think _jobjects should be a nested object rather than a pointer.

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?

src/hotspot/share/jvmci/jvmciRuntime.hpp line 177:

> 175:   int _num_attached_threads;
> 176:   enum {
> 177:     cannot_be_attached = -1 // See _num_attached_threads

We no longer use enums (with unspecified type) to define constants.  Use `static const` instead.  (There's lots of old code that still does this, but new code should not.)

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

src/hotspot/share/runtime/thread.hpp line 1230:

> 1228:   JVMCIRuntime* libjvmci_runtime() const          { return _libjvmci_runtime; }
> 1229:   void set_libjvmci_runtime(JVMCIRuntime* rt) {
> 1230:     assert((_libjvmci_runtime == NULL && rt != NULL) || (_libjvmci_runtime != NULL && rt == NULL), "must be");

Prefer nullptr to NULL in new code.

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

Changes requested by kbarrett (Reviewer).

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