refactor: move the builtin layer's isolate state into RuntimeState

BuiltinLoader kept two Isolate*-keyed process-wide maps (isolateToPrimordials,
isolateToBuiltinRequire) and NsBuiltinModules a third (isolateToRealm), each
behind its own mutex -- the same shape RuntimeState exists to remove. The
primordials lookup runs on every builtin call, so that one took a lock on a hot
path to reach state that was never actually shared.

All three become per-runtime state: a BuiltinRealm holding the two handles as
v8::Globals, and RealmState, which was already a per-runtime struct with a
destructor. Reaching either is now an isolate data-slot read plus a vector
index, and both are released with the runtime while its isolate is alive.

GetRealm can now return null (the runtime has begun tearing down), so its four
callers degrade rather than resurrect state teardown already released;
Instantiate keeps its contract of leaving an exception pending.

With nothing left to release per isolate, disposeIsolate and IsolateDisposer
are deleted along with the DestroyRuntime call site. RealmState and the
BuiltinLoader handles are consequently destroyed at m_state->Clear() instead;
neither destructor runs JS or touches anything torn down in between, and
~RealmState only deletes v8::Persistents, which never call into V8.

Author: edusperoni

test-app/runtime/CMakeLists.txt

@@ -176,7 +176,6 @@ add_library(
         src/main/cpp/FieldAccessor.cpp
         src/main/cpp/File.cpp
         src/main/cpp/Interop.cpp
-        src/main/cpp/IsolateDisposer.cpp
         src/main/cpp/IsolateTracked.cpp
         src/main/cpp/JEnv.cpp
         src/main/cpp/DesugaredInterfaceCompanionClassNameResolver.cpp

test-app/runtime/src/main/cpp/BuiltinLoader.cpp

@@ -5,6 +5,7 @@
 
 #include "ArgConverter.h"
 #include "NsBuiltinModules.h"
+#include "RuntimeState.h"
 #include "robin_hood.h"
 
 using namespace v8;
@@ -36,13 +37,14 @@ constexpr const char* kPrimordialsParamName = "primordials";
 constexpr size_t kParamCount = 5;
 
 /*
- * Per-isolate intrinsics snapshot and builtin require. Worker runtimes
- * initialize on their own threads, so every access is under the mutex.
+ * This runtime's intrinsics snapshot and builtin require. Per-runtime state
+ * rather than an isolate-keyed shared map, so reaching it needs no lock and it
+ * is released with the runtime, while the isolate is still alive.
  */
-std::mutex primordialsMutex;
-robin_hood::unordered_map<Isolate*, Persistent<Object>*> isolateToPrimordials;
-std::mutex builtinRequireMutex;
-robin_hood::unordered_map<Isolate*, Persistent<v8::Function>*> isolateToBuiltinRequire;
+struct BuiltinRealm {
+    v8::Global<v8::Object> primordials;
+    v8::Global<v8::Function> builtinRequire;
+};
 
 /*
  * The `require` every builtin receives: builtin specifiers only, so a builtin
@@ -70,12 +72,13 @@ void BuiltinRequireCallback(const FunctionCallbackInfo<Value>& info) {
 MaybeLocal<v8::Function> GetBuiltinRequire(Local<Context> context) {
     Isolate* isolate = v8::Isolate::GetCurrent();
 
-    {
-        std::lock_guard<std::mutex> lock(builtinRequireMutex);
-        auto it = isolateToBuiltinRequire.find(isolate);
-        if (it != isolateToBuiltinRequire.end()) {
-            return it->second->Get(isolate);
-        }
+    auto* realm = RuntimeState::For<BuiltinRealm>(isolate);
+    if (realm == nullptr) {
+        return MaybeLocal<v8::Function>();
+    }
+
+    if (!realm->builtinRequire.IsEmpty()) {
+        return realm->builtinRequire.Get(isolate);
     }
 
     Local<v8::Function> require;
@@ -85,10 +88,7 @@ MaybeLocal<v8::Function> GetBuiltinRequire(Local<Context> context) {
         return MaybeLocal<v8::Function>();
     }
 
-    {
-        std::lock_guard<std::mutex> lock(builtinRequireMutex);
-        isolateToBuiltinRequire.emplace(isolate, new Persistent<v8::Function>(isolate, require));
-    }
+    realm->builtinRequire.Reset(isolate, require);
     return require;
 }
 
@@ -191,12 +191,13 @@ MaybeLocal<Value> CallBuiltin(Local<Context> context, BuiltinId id, Local<Value>
 MaybeLocal<Object> GetPrimordials(Local<Context> context) {
     Isolate* isolate = v8::Isolate::GetCurrent();
 
-    {
-        std::lock_guard<std::mutex> lock(primordialsMutex);
-        auto it = isolateToPrimordials.find(isolate);
-        if (it != isolateToPrimordials.end()) {
-            return it->second->Get(isolate);
-        }
+    auto* realm = RuntimeState::For<BuiltinRealm>(isolate);
+    if (realm == nullptr) {
+        return MaybeLocal<Object>();
+    }
+
+    if (!realm->primordials.IsEmpty()) {
+        return realm->primordials.Get(isolate);
     }
 
     Local<Value> result;
@@ -207,10 +208,7 @@ MaybeLocal<Object> GetPrimordials(Local<Context> context) {
     }
 
     Local<Object> primordials = result.As<Object>();
-    {
-        std::lock_guard<std::mutex> lock(primordialsMutex);
-        isolateToPrimordials.emplace(isolate, new Persistent<Object>(isolate, primordials));
-    }
+    realm->primordials.Reset(isolate, primordials);
     return primordials;
 }
 
@@ -226,22 +224,4 @@ MaybeLocal<Value> BuiltinLoader::RunBuiltin(Local<Context> context, BuiltinId id
     return CallBuiltin(context, id, binding, primordials);
 }
 
-void BuiltinLoader::onDisposeIsolate(Isolate* isolate) {
-    {
-        std::lock_guard<std::mutex> lock(primordialsMutex);
-        auto it = isolateToPrimordials.find(isolate);
-        if (it != isolateToPrimordials.end()) {
-            delete it->second;
-            isolateToPrimordials.erase(it);
-        }
-    }
-
-    std::lock_guard<std::mutex> lock(builtinRequireMutex);
-    auto it = isolateToBuiltinRequire.find(isolate);
-    if (it != isolateToBuiltinRequire.end()) {
-        delete it->second;
-        isolateToBuiltinRequire.erase(it);
-    }
-}
-
 }  // namespace tns

test-app/runtime/src/main/cpp/BuiltinLoader.h

@@ -27,8 +27,6 @@ class BuiltinLoader {
     static v8::MaybeLocal<v8::Value> RunBuiltin(
             v8::Local<v8::Context> context, BuiltinId id,
             v8::Local<v8::Value> binding = v8::Local<v8::Value>());
-
-    static void onDisposeIsolate(v8::Isolate* isolate);
 };
 
 }  // namespace tns

test-app/runtime/src/main/cpp/IsolateDisposer.cpp

@@ -2,11 +2,11 @@
 
 #include <android/log.h>
 
-#include <mutex>
 #include <vector>
 
 #include "ArgConverter.h"
 #include "BuiltinLoader.h"
+#include "RuntimeState.h"
 #include "console/Console.h"
 #include "robin_hood.h"
 
@@ -50,11 +50,9 @@ bool HasPrefix(const std::string& specifier, const char* prefix) {
 
 /*
  * A builtin module is a singleton per realm, so every cache here is per
- * isolate: workers get their own exports objects and their own synthetic
+ * runtime: workers get their own exports objects and their own synthetic
  * modules. The process-global g_moduleRegistry deliberately holds none of
- * this. Worker runtimes initialize on their own threads, so the map itself is
- * under the mutex; the state it holds is only ever touched from its isolate's
- * thread.
+ * this. Touched only from its own runtime's thread.
  */
 struct RealmState {
     robin_hood::unordered_map<std::string, Persistent<Object>*> exports;
@@ -76,16 +74,13 @@ struct RealmState {
     }
 };
 
-std::mutex realmsMutex;
-robin_hood::unordered_map<Isolate*, RealmState*> isolateToRealm;
-
-RealmState& GetRealm(Isolate* isolate) {
-    std::lock_guard<std::mutex> lock(realmsMutex);
-    auto it = isolateToRealm.find(isolate);
-    if (it == isolateToRealm.end()) {
-        it = isolateToRealm.emplace(isolate, new RealmState()).first;
-    }
-    return *it->second;
+/*
+ * This runtime's realm. Per-runtime state rather than an isolate-keyed shared
+ * map, so reaching it needs no lock and it is released with the runtime. Null
+ * once the runtime has begun tearing down.
+ */
+RealmState* GetRealm(Isolate* isolate) {
+    return RuntimeState::For<RealmState>(isolate);
 }
 
 MaybeLocal<Object> BuildBinding(Local<Context> context, BuiltinId builtin) {
@@ -119,7 +114,13 @@ MaybeLocal<Object> BuildBinding(Local<Context> context, BuiltinId builtin) {
  */
 bool Instantiate(Local<Context> context, const Registration& requested) {
     Isolate* isolate = v8::Isolate::GetCurrent();
-    RealmState& realm = GetRealm(isolate);
+    RealmState* realmState = GetRealm(isolate);
+    if (realmState == nullptr) {
+        isolate->ThrowException(Exception::Error(ArgConverter::ConvertToV8String(
+                isolate, "Cannot load a builtin module: the runtime is shutting down")));
+        return false;
+    }
+    RealmState& realm = *realmState;
 
     /*
      * A shim reaches its ns: module through the builtin require, so the graph
@@ -241,7 +242,11 @@ MaybeLocal<Object> NsBuiltinModules::GetExports(Local<Context> context,
     }
 
     Isolate* isolate = v8::Isolate::GetCurrent();
-    RealmState& realm = GetRealm(isolate);
+    RealmState* realmState = GetRealm(isolate);
+    if (realmState == nullptr) {
+        return MaybeLocal<Object>();
+    }
+    RealmState& realm = *realmState;
     auto it = realm.exports.find(specifier);
     if (it == realm.exports.end()) {
         if (!Instantiate(context, *registration)) {
@@ -258,7 +263,11 @@ MaybeLocal<Object> NsBuiltinModules::GetExports(Local<Context> context,
 MaybeLocal<Module> NsBuiltinModules::GetModule(Local<Context> context,
                                                const std::string& specifier) {
     Isolate* isolate = v8::Isolate::GetCurrent();
-    RealmState& realm = GetRealm(isolate);
+    RealmState* realmState = GetRealm(isolate);
+    if (realmState == nullptr) {
+        return MaybeLocal<Module>();
+    }
+    RealmState& realm = *realmState;
 
     auto it = realm.modules.find(specifier);
     if (it != realm.modules.end()) {
@@ -310,7 +319,11 @@ MaybeLocal<Module> NsBuiltinModules::GetModule(Local<Context> context,
 
 Local<v8::Function> NsBuiltinModules::GetFormatFunc(Local<Context> context) {
     Isolate* isolate = v8::Isolate::GetCurrent();
-    RealmState& realm = GetRealm(isolate);
+    RealmState* realmState = GetRealm(isolate);
+    if (realmState == nullptr) {
+        return Local<v8::Function>();
+    }
+    RealmState& realm = *realmState;
     if (realm.format != nullptr) {
         return realm.format->Get(isolate);
     }
@@ -338,13 +351,4 @@ Local<v8::Function> NsBuiltinModules::GetFormatFunc(Local<Context> context) {
     return format.As<v8::Function>();
 }
 
-void NsBuiltinModules::onDisposeIsolate(Isolate* isolate) {
-    std::lock_guard<std::mutex> lock(realmsMutex);
-    auto it = isolateToRealm.find(isolate);
-    if (it != isolateToRealm.end()) {
-        delete it->second;
-        isolateToRealm.erase(it);
-    }
-}
-
 }  // namespace tns

test-app/runtime/src/main/cpp/IsolateDisposer.h

@@ -57,8 +57,6 @@ class NsBuiltinModules {
      * module could not be built; callers degrade instead of failing the log.
      */
     static v8::Local<v8::Function> GetFormatFunc(v8::Local<v8::Context> context);
-
-    static void onDisposeIsolate(v8::Isolate* isolate);
 };
 
 }  // namespace tns

test-app/runtime/src/main/cpp/NsBuiltinModules.cpp

@@ -21,7 +21,6 @@
 #include "FrameCallbacks.h"
 #include "Interop.h"
 #include "IsolateTracked.h"
-#include "IsolateDisposer.h"
 #include "JType.h"
 #include "JsArgConverter.h"
 #include "JsArgToArrayConverter.h"
@@ -1023,10 +1022,10 @@ void Runtime::DestroyRuntime() {
     m_eventLoop->Shutdown();
   }
   if (m_napiEnv != nullptr) {
-    // After Shutdown so no queued Node-API entry can run against a dying env,
-    // and before disposeIsolate: the env's reference lists hold v8::Globals,
-    // so its teardown needs the isolate alive and locked. The Locker is
-    // reentrant for the worker path, which already holds it here.
+    // After Shutdown so no queued Node-API entry can run against a dying env.
+    // The env's reference lists hold v8::Globals, so its teardown needs the
+    // isolate alive and locked; the Locker is reentrant for the worker path,
+    // which already holds it here.
     v8::Locker locker(m_isolate);
     NapiEnv::Destroy(static_cast<NapiEnv*>(m_napiEnv));
     m_napiEnv = nullptr;
@@ -1052,8 +1051,6 @@ void Runtime::DestroyRuntime() {
   CallbackHandlers::RemoveIsolateEntries(m_isolate);
   FrameCallbacks::RemoveIsolateEntries(m_isolate);
 
-  tns::disposeIsolate(m_isolate);
-
   // V8 does not run weak callbacks when an isolate is disposed, so anything
   // still bound to one has to be deleted explicitly, here, while the isolate
   // is alive and its destructors can still touch v8::Global handles.

test-app/runtime/src/main/cpp/NsBuiltinModules.h

@@ -74,8 +74,8 @@ module.exports = somethingTheCallSiteNeeds;
 
 `primordials.js` runs first in every isolate — lazily, on the first
 `RunBuiltin` call, which happens during runtime init — and its frozen,
-null-prototype export is cached per isolate (`BuiltinLoader`, released from
-`disposeIsolate`) and handed to every other builtin, so a builtin that
+null-prototype export is cached per runtime (`BuiltinLoader`, in
+`RuntimeState`) and handed to every other builtin, so a builtin that
 compiles later in the isolate's life still sees intrinsics as they were before
 user code ran.