src: make BuiltinLoader threadsafe and non-global

As discussed in nodejs/node#45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.

PR-URL: nodejs/node#45942
Reviewed-By: Joyee Cheung <[email protected]>

Author: sercher

Diff for: graal-nodejs/src/api/environment.cc

@@ -492,7 +492,7 @@ MaybeLocal<Value> LoadEnvironment(
   return LoadEnvironment(
       env, [&](const StartExecutionCallbackInfo& info) -> MaybeLocal<Value> {
         std::string name = "embedder_main_" + std::to_string(env->thread_id());
-        builtins::BuiltinLoader::Add(name.c_str(), main_script_source_utf8);
+        env->builtin_loader()->Add(name.c_str(), main_script_source_utf8);
         Realm* realm = env->principal_realm();
 
         return realm->ExecuteBootstrapper(name.c_str());
@@ -732,10 +732,17 @@ Maybe<bool> InitializePrimordials(Local<Context> context) {
                                         "internal/per_context/messageport",
                                         nullptr};
 
+  // We do not have access to a per-Environment BuiltinLoader instance
+  // at this point, because this code runs before an Environment exists
+  // in the first place. However, creating BuiltinLoader instances is
+  // relatively cheap and all the scripts that we may want to run at
+  // startup are always present in it.
+  thread_local builtins::BuiltinLoader builtin_loader;
   for (const char** module = context_files; *module != nullptr; module++) {
     Local<Value> arguments[] = {exports, primordials};
-    if (builtins::BuiltinLoader::CompileAndCall(
-            context, *module, arraysize(arguments), arguments, nullptr)
+    if (builtin_loader
+            .CompileAndCall(
+                context, *module, arraysize(arguments), arguments, nullptr)
             .IsEmpty()) {
       // Execution failed during context creation.
       return Nothing<bool>();

Diff for: graal-nodejs/src/env-inl.h

@@ -388,6 +388,10 @@ inline std::vector<double>* Environment::destroy_async_id_list() {
   return &destroy_async_id_list_;
 }
 
+inline builtins::BuiltinLoader* Environment::builtin_loader() {
+  return &builtin_loader_;
+}
+
 inline double Environment::new_async_id() {
   async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter] += 1;
   return async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter];

Diff for: graal-nodejs/src/env.cc

@@ -676,6 +676,15 @@ Environment::Environment(IsolateData* isolate_data,
       thread_id_(thread_id.id == static_cast<uint64_t>(-1)
                      ? AllocateEnvironmentThreadId().id
                      : thread_id.id) {
+#ifdef NODE_V8_SHARED_RO_HEAP
+  if (!is_main_thread()) {
+    CHECK_NOT_NULL(isolate_data->worker_context());
+    // TODO(addaleax): Adjust for the embedder API snapshot support changes
+    builtin_loader()->CopySourceAndCodeCacheReferenceFrom(
+        isolate_data->worker_context()->env()->builtin_loader());
+  }
+#endif
+
   // We'll be creating new objects so make sure we've entered the context.
   HandleScope handle_scope(isolate);
 

Diff for: graal-nodejs/src/env.h

@@ -695,6 +695,8 @@ class Environment : public MemoryRetainer {
   // List of id's that have been destroyed and need the destroy() cb called.
   inline std::vector<double>* destroy_async_id_list();
 
+  builtins::BuiltinLoader* builtin_loader();
+
   std::unordered_multimap<int, loader::ModuleWrap*> hash_to_module_map;
   std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
   std::unordered_map<uint32_t, contextify::ContextifyScript*>
@@ -1099,6 +1101,8 @@ class Environment : public MemoryRetainer {
 
   std::unique_ptr<Realm> principal_realm_ = nullptr;
 
+  builtins::BuiltinLoader builtin_loader_;
+
   // Used by allocate_managed_buffer() and release_managed_buffer() to keep
   // track of the BackingStore for a given pointer.
   std::unordered_map<char*, std::unique_ptr<v8::BackingStore>>

Diff for: graal-nodejs/src/node.cc

@@ -132,8 +132,6 @@
 
 namespace node {
 
-using builtins::BuiltinLoader;
-
 using v8::EscapableHandleScope;
 using v8::Isolate;
 using v8::Local;
@@ -1231,9 +1229,6 @@ int LoadSnapshotDataAndRun(const SnapshotData** snapshot_data_ptr,
     }
   }
 
-  if ((*snapshot_data_ptr) != nullptr) {
-    BuiltinLoader::RefreshCodeCache((*snapshot_data_ptr)->code_cache);
-  }
   NodeMainInstance main_instance(*snapshot_data_ptr,
                                  uv_default_loop(),
                                  per_process::v8_platform.Platform(),

Diff for: graal-nodejs/src/node_binding.cc

@@ -663,13 +663,13 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
     CHECK(exports->SetPrototype(context, Null(isolate)).FromJust());
     DefineConstants(isolate, exports);
   } else if (!strcmp(*module_v, "natives")) {
-    exports = builtins::BuiltinLoader::GetSourceObject(context);
+    exports = realm->env()->builtin_loader()->GetSourceObject(context);
     // Legacy feature: process.binding('natives').config contains stringified
     // config.gypi
     CHECK(exports
               ->Set(context,
                     realm->isolate_data()->config_string(),
-                    builtins::BuiltinLoader::GetConfigString(isolate))
+                    realm->env()->builtin_loader()->GetConfigString(isolate))
               .FromJust());
   } else {
     return THROW_ERR_INVALID_MODULE(isolate, "No such binding: %s", *module_v);