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

[Kim Barrett]

On Tue, 19 Apr 2022 10:17:14 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.
>
> Doug Simon has updated the pull request incrementally with one additional commit since the last revision:
> 
>   removed (incorrect) volatile modifier from JVMCIRuntime::_shared_library_javavm

One more possible improvement suggestion.  As I said before, I've only reviewed the runtime parts (which looks okay), and just skimmed the rest.  So don't count me as the required 2nd reviewer.

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

> 893:     ResourceMark rm;
> 894:     oop** ptrs = NEW_RESOURCE_ARRAY(oop*, _oop_handles.length());
> 895:     for (int i = 0; i < _oop_handles.length(); i++) {

Now that _oop_handles is a vector of `oop*`, the resource allocation and
possibly some copying could be avoided. Do an in-place removal of nullptr and
clearing of the values pass over the vector, and then use

object_handles()->release(_oop_handles.at_ptr(0), _oop_handles.length())

For future, I wonder if it would be worth changing OopStorage bulk release to ignore nullptrs in the array, to better support this kind of use-case.  But I'm not sure how common this kind of use-case (array of handles, some of which might be nullptr) really might be.  I think there are only a couple uses of bulk release so far.

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

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