From 42f9b9384f341cdff99e2d8079769bd71fa06cc9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 22 Dec 2022 18:40:01 +0100 Subject: [PATCH] src: make BuiltinLoader threadsafe and non-global As discussed in https://github.com/nodejs/node/pull/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. --- src/api/environment.cc | 6 +- src/env-inl.h | 9 ++ src/env.cc | 2 + src/env.h | 5 + src/node.cc | 3 - src/node_binding.cc | 4 +- src/node_builtins.cc | 163 ++++++++++++++++---------------- src/node_builtins.h | 62 ++++++------ src/node_main_instance.cc | 5 + src/node_realm.cc | 2 +- src/node_snapshotable.cc | 4 +- src/node_worker.cc | 2 + test/cctest/test_per_process.cc | 2 +- 13 files changed, 147 insertions(+), 122 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 529929deb63524..dbca7f8cfddb45 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -458,9 +458,8 @@ MaybeLocal LoadEnvironment( main_script_source_utf8).ToLocalChecked(); auto main_utf16 = std::make_unique(isolate, str); - // TODO(addaleax): Avoid having a global table for all scripts. std::string name = "embedder_main_" + std::to_string(env->thread_id()); - builtins::BuiltinLoader::Add( + env->builtin_loader()->Add( name.c_str(), UnionBytes(**main_utf16, main_utf16->length())); env->set_main_utf16(std::move(main_utf16)); Realm* realm = env->principal_realm(); @@ -703,9 +702,10 @@ Maybe InitializePrimordials(Local context) { "internal/per_context/messageport", nullptr}; + auto builtin_loader = builtins::BuiltinLoader::Create(); for (const char** module = context_files; *module != nullptr; module++) { Local arguments[] = {exports, primordials}; - if (builtins::BuiltinLoader::CompileAndCall( + if (builtin_loader->CompileAndCall( context, *module, arraysize(arguments), arguments, nullptr) .IsEmpty()) { // Execution failed during context creation. diff --git a/src/env-inl.h b/src/env-inl.h index a512d175a29cfe..9c6d98170396b6 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -432,6 +432,15 @@ inline std::vector* Environment::destroy_async_id_list() { return &destroy_async_id_list_; } +inline std::shared_ptr Environment::builtin_loader() { + DCHECK(builtin_loader_); + return builtin_loader_; +} + +inline void Environment::set_builtin_loader(std::shared_ptr loader) { + builtin_loader_ = loader; +} + inline double Environment::new_async_id() { async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter] += 1; return async_hooks()->async_id_fields()[AsyncHooks::kAsyncIdCounter]; diff --git a/src/env.cc b/src/env.cc index 97e0ac58111303..80df558e39163e 100644 --- a/src/env.cc +++ b/src/env.cc @@ -747,6 +747,8 @@ Environment::Environment(IsolateData* isolate_data, env_info, flags, thread_id) { + // TODO(addaleax): Make this part of CreateEnvironment(). + set_builtin_loader(builtins::BuiltinLoader::Create()); InitializeMainContext(context, env_info); } diff --git a/src/env.h b/src/env.h index 396416b42ae348..eda5c78e3bf1f9 100644 --- a/src/env.h +++ b/src/env.h @@ -716,6 +716,8 @@ class Environment : public MemoryRetainer { inline std::vector* destroy_async_id_list(); std::set internal_bindings; + std::shared_ptr builtin_loader(); + void set_builtin_loader(std::shared_ptr loader); std::set builtins_with_cache; std::set builtins_without_cache; // This is only filled during deserialization. We use a vector since @@ -1147,7 +1149,10 @@ class Environment : public MemoryRetainer { // Keeps the main script source alive is one was passed to LoadEnvironment(). // We should probably find a way to just use plain `v8::String`s created from // the source passed to LoadEnvironment() directly instead. + // TODO(addaleax): Move this to BuiltinLoader, like we do for + // BuiltinLoader::AddExternalizedBuiltin(). std::unique_ptr main_utf16_; + std::shared_ptr builtin_loader_; // Used by allocate_managed_buffer() and release_managed_buffer() to keep // track of the BackingStore for a given pointer. diff --git a/src/node.cc b/src/node.cc index 3da63b5d511531..e77265a46cb2ea 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1183,9 +1183,6 @@ ExitCode 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 --git a/src/node_binding.cc b/src/node_binding.cc index ab25501dcbae96..4dc860675f43de 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -586,13 +586,13 @@ void GetInternalBinding(const FunctionCallbackInfo& args) { exports->SetPrototype(env->context(), Null(env->isolate())).FromJust()); DefineConstants(env->isolate(), exports); } else if (!strcmp(*module_v, "natives")) { - exports = builtins::BuiltinLoader::GetSourceObject(env->context()); + exports = env->builtin_loader()->GetSourceObject(env->context()); // Legacy feature: process.binding('natives').config contains stringified // config.gypi CHECK(exports ->Set(env->context(), env->config_string(), - builtins::BuiltinLoader::GetConfigString(env->isolate())) + env->builtin_loader()->GetConfigString(env->isolate())) .FromJust()); } else { return THROW_ERR_INVALID_MODULE(env, "No such binding: %s", *module_v); diff --git a/src/node_builtins.cc b/src/node_builtins.cc index 77fa5741de43d6..40d3fd39b7e8da 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -29,8 +29,6 @@ using v8::String; using v8::Undefined; using v8::Value; -BuiltinLoader BuiltinLoader::instance_; - BuiltinLoader::BuiltinLoader() : config_(GetConfig()), has_code_cache_(false) { LoadJavaScriptSource(); #if defined(NODE_HAVE_I18N_SUPPORT) @@ -53,25 +51,24 @@ BuiltinLoader::BuiltinLoader() : config_(GetConfig()), has_code_cache_(false) { #endif // NODE_HAVE_I18N_SUPPORT } -BuiltinLoader* BuiltinLoader::GetInstance() { - return &instance_; -} - bool BuiltinLoader::Exists(const char* id) { - auto& source = GetInstance()->source_; - return source.find(id) != source.end(); + RwLock::ScopedReadLock lock(source_mutex_); + return source_.find(id) != source_.end(); } bool BuiltinLoader::Add(const char* id, const UnionBytes& source) { - auto result = GetInstance()->source_.emplace(id, source); + RwLock::ScopedLock source_lock(source_mutex_); + Mutex::ScopedLock categories_lock(builtin_categories_mutex_); + builtin_categories_.reset(); // The category cache is reset by adding new sources + auto result = source_.emplace(id, source); return result.second; } Local BuiltinLoader::GetSourceObject(Local context) { + RwLock::ScopedReadLock lock(source_mutex_); Isolate* isolate = context->GetIsolate(); Local out = Object::New(isolate); - auto& source = GetInstance()->source_; - for (auto const& x : source) { + for (auto const& x : source_) { Local key = OneByteString(isolate, x.first.c_str(), x.first.size()); out->Set(context, key, x.second.ToStringChecked(isolate)).FromJust(); } @@ -79,10 +76,11 @@ Local BuiltinLoader::GetSourceObject(Local context) { } Local BuiltinLoader::GetConfigString(Isolate* isolate) { - return GetInstance()->config_.ToStringChecked(isolate); + return config_.ToStringChecked(isolate); } -std::vector BuiltinLoader::GetBuiltinIds() { +std::vector BuiltinLoader::GetBuiltinIds() const { + RwLock::ScopedReadLock lock(source_mutex_); std::vector ids; ids.reserve(source_.size()); for (auto const& x : source_) { @@ -91,11 +89,13 @@ std::vector BuiltinLoader::GetBuiltinIds() { return ids; } -void BuiltinLoader::InitializeBuiltinCategories() { - if (builtin_categories_.is_initialized) { - DCHECK(!builtin_categories_.can_be_required.empty()); - return; +const BuiltinLoader::BuiltinCategories& BuiltinLoader::InitializeBuiltinCategories() const { + Mutex::ScopedLock lock(builtin_categories_mutex_); + if (LIKELY(builtin_categories_.has_value())) { + DCHECK(!builtin_categories_.value().can_be_required.empty()); + return builtin_categories_.value(); } + auto& builtin_categories = builtin_categories_.emplace(); std::vector prefixes = { #if !HAVE_OPENSSL @@ -109,10 +109,10 @@ void BuiltinLoader::InitializeBuiltinCategories() { "internal/main/" }; - builtin_categories_.can_be_required.emplace( + builtin_categories.can_be_required.emplace( "internal/deps/cjs-module-lexer/lexer"); - builtin_categories_.cannot_be_required = std::set { + builtin_categories.cannot_be_required = std::set { #if !HAVE_INSPECTOR "inspector", "inspector/promises", "internal/util/inspector", #endif // !HAVE_INSPECTOR @@ -142,46 +142,40 @@ void BuiltinLoader::InitializeBuiltinCategories() { continue; } if (id.find(prefix) == 0 && - builtin_categories_.can_be_required.count(id) == 0) { - builtin_categories_.cannot_be_required.emplace(id); + builtin_categories.can_be_required.count(id) == 0) { + builtin_categories.cannot_be_required.emplace(id); } } } for (auto const& x : source_) { const std::string& id = x.first; - if (0 == builtin_categories_.cannot_be_required.count(id)) { - builtin_categories_.can_be_required.emplace(id); + if (0 == builtin_categories.cannot_be_required.count(id)) { + builtin_categories.can_be_required.emplace(id); } } - builtin_categories_.is_initialized = true; + return builtin_categories; } -const std::set& BuiltinLoader::GetCannotBeRequired() { - InitializeBuiltinCategories(); - return builtin_categories_.cannot_be_required; +const std::set& BuiltinLoader::GetCannotBeRequired() const { + return InitializeBuiltinCategories().cannot_be_required; } -const std::set& BuiltinLoader::GetCanBeRequired() { - InitializeBuiltinCategories(); - return builtin_categories_.can_be_required; +const std::set& BuiltinLoader::GetCanBeRequired() const { + return InitializeBuiltinCategories().can_be_required; } -bool BuiltinLoader::CanBeRequired(const char* id) { +bool BuiltinLoader::CanBeRequired(const char* id) const { return GetCanBeRequired().count(id) == 1; } -bool BuiltinLoader::CannotBeRequired(const char* id) { +bool BuiltinLoader::CannotBeRequired(const char* id) const { return GetCannotBeRequired().count(id) == 1; } -BuiltinCodeCacheMap* BuiltinLoader::code_cache() { - return &code_cache_; -} - ScriptCompiler::CachedData* BuiltinLoader::GetCodeCache(const char* id) const { - Mutex::ScopedLock lock(code_cache_mutex_); + RwLock::ScopedReadLock lock(code_cache_mutex_); const auto it = code_cache_.find(id); if (it == code_cache_.end()) { // The module has not been compiled before. @@ -208,10 +202,11 @@ static std::string OnDiskFileName(const char* id) { #endif // NODE_BUILTIN_MODULES_PATH MaybeLocal BuiltinLoader::LoadBuiltinSource(Isolate* isolate, - const char* id) { + const char* id) const { #ifdef NODE_BUILTIN_MODULES_PATH if (strncmp(id, "embedder_main_", strlen("embedder_main_")) == 0) { #endif // NODE_BUILTIN_MODULES_PATH + RwLock::ScopedReadLock lock(source_mutex_); const auto source_it = source_.find(id); if (UNLIKELY(source_it == source_.end())) { fprintf(stderr, "Cannot find native builtin: \"%s\".\n", id); @@ -239,15 +234,29 @@ MaybeLocal BuiltinLoader::LoadBuiltinSource(Isolate* isolate, } #if defined(NODE_HAVE_I18N_SUPPORT) +namespace { +static Mutex externalized_builtins_mutex; +std::unordered_map externalized_builtin_sources; +} + void BuiltinLoader::AddExternalizedBuiltin(const char* id, const char* filename) { std::string source; - int r = ReadFileSync(&source, filename); - if (r != 0) { - fprintf( - stderr, "Cannot load externalized builtin: \"%s:%s\".\n", id, filename); - ABORT(); - return; + { + Mutex::ScopedLock lock(externalized_builtins_mutex); + auto it = externalized_builtin_sources.find(id); + if (it != externalized_builtin_sources.end()) { + source = it->second; + } { + int r = ReadFileSync(&source, filename); + if (r != 0) { + fprintf( + stderr, "Cannot load externalized builtin: \"%s:%s\".\n", id, filename); + ABORT(); + return; + } + externalized_builtin_sources[id] = source; + } } icu::UnicodeString utf16 = icu::UnicodeString::fromUTF8( @@ -257,7 +266,7 @@ void BuiltinLoader::AddExternalizedBuiltin(const char* id, UnionBytes(reinterpret_cast((*source_utf16).getBuffer()), utf16.length())); // keep source bytes for builtin alive while BuiltinLoader exists - GetInstance()->externalized_source_bytes_.push_back(std::move(source_utf16)); + externalized_source_bytes_.push_back(std::move(source_utf16)); } #endif // NODE_HAVE_I18N_SUPPORT @@ -288,7 +297,7 @@ MaybeLocal BuiltinLoader::LookupAndCompileInternal( // `CompileFunction()` call below, because this function may recurse if // there is a syntax error during bootstrap (because the fatal exception // handler is invoked, which may load built-in modules). - Mutex::ScopedLock lock(code_cache_mutex_); + RwLock::ScopedLock lock(code_cache_mutex_); auto cache_it = code_cache_.find(id); if (cache_it != code_cache_.end()) { // Transfer ownership to ScriptCompiler::Source later. @@ -354,15 +363,8 @@ MaybeLocal BuiltinLoader::LookupAndCompileInternal( CHECK_NOT_NULL(new_cached_data); { - Mutex::ScopedLock lock(code_cache_mutex_); - const auto it = code_cache_.find(id); - // TODO(joyeecheung): it's safer for each thread to have its own - // copy of the code cache map. - if (it == code_cache_.end()) { - code_cache_.emplace(id, std::move(new_cached_data)); - } else { - it->second.reset(new_cached_data.release()); - } + RwLock::ScopedLock lock(code_cache_mutex_); + code_cache_[id] = std::move(new_cached_data); } return scope.Escape(fun); @@ -424,9 +426,10 @@ MaybeLocal BuiltinLoader::LookupAndCompile( }; } - MaybeLocal maybe = GetInstance()->LookupAndCompileInternal( + MaybeLocal maybe = LookupAndCompileInternal( context, id, ¶meters, &result); if (optional_env != nullptr) { + DCHECK_EQ(this, optional_env->builtin_loader().get()); RecordResult(id, result, optional_env); } return maybe; @@ -503,8 +506,7 @@ MaybeLocal BuiltinLoader::CompileAndCall(Local context, } bool BuiltinLoader::CompileAllBuiltins(Local context) { - BuiltinLoader* loader = GetInstance(); - std::vector ids = loader->GetBuiltinIds(); + std::vector ids = GetBuiltinIds(); bool all_succeeded = true; std::string v8_tools_prefix = "internal/deps/v8/tools/"; for (const auto& id : ids) { @@ -512,7 +514,7 @@ bool BuiltinLoader::CompileAllBuiltins(Local context) { continue; } v8::TryCatch bootstrapCatch(context->GetIsolate()); - USE(loader->LookupAndCompile(context, id.c_str(), nullptr)); + USE(LookupAndCompile(context, id.c_str(), nullptr)); if (bootstrapCatch.HasCaught()) { per_process::Debug(DebugCategory::CODE_CACHE, "Failed to compile code cache for %s\n", @@ -525,10 +527,8 @@ bool BuiltinLoader::CompileAllBuiltins(Local context) { } void BuiltinLoader::CopyCodeCache(std::vector* out) { - BuiltinLoader* loader = GetInstance(); - Mutex::ScopedLock lock(loader->code_cache_mutex()); - auto in = loader->code_cache(); - for (auto const& item : *in) { + RwLock::ScopedReadLock lock(code_cache_mutex_); + for (auto const& item : code_cache_) { out->push_back( {item.first, {item.second->data, item.second->data + item.second->length}}); @@ -536,24 +536,16 @@ void BuiltinLoader::CopyCodeCache(std::vector* out) { } void BuiltinLoader::RefreshCodeCache(const std::vector& in) { - BuiltinLoader* loader = GetInstance(); - Mutex::ScopedLock lock(loader->code_cache_mutex()); - auto out = loader->code_cache(); + RwLock::ScopedLock lock(code_cache_mutex_); for (auto const& item : in) { size_t length = item.data.size(); uint8_t* buffer = new uint8_t[length]; memcpy(buffer, item.data.data(), length); auto new_cache = std::make_unique( buffer, length, v8::ScriptCompiler::CachedData::BufferOwned); - auto cache_it = out->find(item.id); - if (cache_it != out->end()) { - // Release the old cache and replace it with the new copy. - cache_it->second.reset(new_cache.release()); - } else { - out->emplace(item.id, new_cache.release()); - } + code_cache_[item.id] = std::move(new_cache); } - loader->has_code_cache_ = true; + has_code_cache_ = true; } void BuiltinLoader::GetBuiltinCategories( @@ -565,8 +557,8 @@ void BuiltinLoader::GetBuiltinCategories( // Copy from the per-process categories std::set cannot_be_required = - GetInstance()->GetCannotBeRequired(); - std::set can_be_required = GetInstance()->GetCanBeRequired(); + env->builtin_loader()->GetCannotBeRequired(); + std::set can_be_required = env->builtin_loader()->GetCanBeRequired(); if (!env->owns_process_state()) { can_be_required.erase("trace_events"); @@ -645,16 +637,19 @@ void BuiltinLoader::GetCacheUsage(const FunctionCallbackInfo& args) { void BuiltinLoader::BuiltinIdsGetter(Local property, const PropertyCallbackInfo& info) { - Isolate* isolate = info.GetIsolate(); + Environment* env = Environment::GetCurrent(info); + Isolate* isolate = env->isolate(); - std::vector ids = GetInstance()->GetBuiltinIds(); + std::vector ids = env->builtin_loader()->GetBuiltinIds(); info.GetReturnValue().Set( ToV8Value(isolate->GetCurrentContext(), ids).ToLocalChecked()); } void BuiltinLoader::ConfigStringGetter( Local property, const PropertyCallbackInfo& info) { - info.GetReturnValue().Set(GetConfigString(info.GetIsolate())); + Environment* env = Environment::GetCurrent(info); + info.GetReturnValue().Set( + env->builtin_loader()->GetConfigString(info.GetIsolate())); } void BuiltinLoader::RecordResult(const char* id, @@ -673,7 +668,7 @@ void BuiltinLoader::CompileFunction(const FunctionCallbackInfo& args) { node::Utf8Value id_v(env->isolate(), args[0].As()); const char* id = *id_v; MaybeLocal maybe = - GetInstance()->LookupAndCompile(env->context(), id, env); + env->builtin_loader()->LookupAndCompile(env->context(), id, env); Local fn; if (maybe.ToLocal(&fn)) { args.GetReturnValue().Set(fn); @@ -681,8 +676,14 @@ void BuiltinLoader::CompileFunction(const FunctionCallbackInfo& args) { } void BuiltinLoader::HasCachedBuiltins(const FunctionCallbackInfo& args) { + auto instance = Environment::GetCurrent(args)->builtin_loader(); + RwLock::ScopedReadLock lock(instance->code_cache_mutex_); args.GetReturnValue().Set( - v8::Boolean::New(args.GetIsolate(), GetInstance()->has_code_cache_)); + v8::Boolean::New(args.GetIsolate(), instance->has_code_cache_)); +} + +std::shared_ptr BuiltinLoader::Create() { + return std::shared_ptr { new BuiltinLoader() }; } // TODO(joyeecheung): It is somewhat confusing that Class::Initialize diff --git a/src/node_builtins.h b/src/node_builtins.h index 42338350765874..4a576e3023aef8 100644 --- a/src/node_builtins.h +++ b/src/node_builtins.h @@ -12,6 +12,7 @@ #include #endif // NODE_HAVE_I18N_SUPPORT #include +#include #include "node_mutex.h" #include "node_union_bytes.h" #include "v8.h" @@ -51,63 +52,60 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { // The parameters used to compile the scripts are detected based on // the pattern of the id. - static v8::MaybeLocal LookupAndCompile( + v8::MaybeLocal LookupAndCompile( v8::Local context, const char* id, Environment* optional_env); - static v8::MaybeLocal CompileAndCall( + v8::MaybeLocal CompileAndCall( v8::Local context, const char* id, int argc, v8::Local argv[], Environment* optional_env); - static v8::MaybeLocal CompileAndCall( + v8::MaybeLocal CompileAndCall( v8::Local context, const char* id, Realm* realm); - static v8::Local GetSourceObject(v8::Local context); + v8::Local GetSourceObject(v8::Local context); // Returns config.gypi as a JSON string - static v8::Local GetConfigString(v8::Isolate* isolate); - static bool Exists(const char* id); - static bool Add(const char* id, const UnionBytes& source); + v8::Local GetConfigString(v8::Isolate* isolate); + bool Exists(const char* id); + bool Add(const char* id, const UnionBytes& source); - static bool CompileAllBuiltins(v8::Local context); - static void RefreshCodeCache(const std::vector& in); - static void CopyCodeCache(std::vector* out); + bool CompileAllBuiltins(v8::Local context); + void RefreshCodeCache(const std::vector& in); + void CopyCodeCache(std::vector* out); + + static std::shared_ptr Create(); private: // Only allow access from friends. friend class CodeCacheBuilder; BuiltinLoader(); - static BuiltinLoader* GetInstance(); // Generated by tools/js2c.py as node_javascript.cc void LoadJavaScriptSource(); // Loads data into source_ UnionBytes GetConfig(); // Return data for config.gypi - std::vector GetBuiltinIds(); + std::vector GetBuiltinIds() const; struct BuiltinCategories { - bool is_initialized = false; std::set can_be_required; std::set cannot_be_required; }; - void InitializeBuiltinCategories(); - const std::set& GetCannotBeRequired(); - const std::set& GetCanBeRequired(); - - bool CanBeRequired(const char* id); - bool CannotBeRequired(const char* id); + const BuiltinCategories& InitializeBuiltinCategories() const; + const std::set& GetCannotBeRequired() const; + const std::set& GetCanBeRequired() const; - BuiltinCodeCacheMap* code_cache(); - const Mutex& code_cache_mutex() const { return code_cache_mutex_; } + bool CanBeRequired(const char* id) const; + bool CannotBeRequired(const char* id) const; v8::ScriptCompiler::CachedData* GetCodeCache(const char* id) const; enum class Result { kWithCache, kWithoutCache }; v8::MaybeLocal LoadBuiltinSource(v8::Isolate* isolate, - const char* id); + const char* id) const; // If an exception is encountered (e.g. source code contains // syntax error), the returned value is empty. v8::MaybeLocal LookupAndCompileInternal( @@ -137,21 +135,27 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { const v8::FunctionCallbackInfo& args); #if defined(NODE_HAVE_I18N_SUPPORT) - static void AddExternalizedBuiltin(const char* id, const char* filename); + void AddExternalizedBuiltin(const char* id, const char* filename); #endif // NODE_HAVE_I18N_SUPPORT - static BuiltinLoader instance_; - BuiltinCategories builtin_categories_; + // Used to synchronize access to builtin_categories. + // builtin_categories is marked mutable because it is only initialized + // as a cache, so that mutating it does not change any state. + Mutex builtin_categories_mutex_; + mutable std::optional builtin_categories_; + + // Used to synchronize access to the code cache map + RwLock source_mutex_; BuiltinSourceMap source_; - BuiltinCodeCacheMap code_cache_; - UnionBytes config_; #if defined(NODE_HAVE_I18N_SUPPORT) std::list> externalized_source_bytes_; #endif // NODE_HAVE_I18N_SUPPORT - // Used to synchronize access to the code cache map - Mutex code_cache_mutex_; + const UnionBytes config_; + // Used to synchronize access to the code cache map + RwLock code_cache_mutex_; + BuiltinCodeCacheMap code_cache_; bool has_code_cache_; friend class ::PerProcessTest; diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 5060c69499f813..598203b985a6d2 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -169,6 +169,11 @@ NodeMainInstance::CreateMainEnvironment(ExitCode* exit_code) { &(snapshot_data_->env_info), EnvironmentFlags::kDefaultFlags, {})); + // TODO(addaleax): Do this as part of creating the Environment + // once we store the SnapshotData* itself on IsolateData. + auto builtin_loader = builtins::BuiltinLoader::Create(); + builtin_loader->RefreshCodeCache(snapshot_data_->code_cache); + env->set_builtin_loader(builtin_loader); context = Context::FromSnapshot(isolate_, SnapshotData::kNodeMainContextIndex, {DeserializeNodeInternalFields, env.get()}) diff --git a/src/node_realm.cc b/src/node_realm.cc index 182fc9b2c4b249..fd9f5cfae9c0ea 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -165,7 +165,7 @@ void Realm::DeserializeProperties(const RealmSerializeInfo* info) { MaybeLocal Realm::ExecuteBootstrapper(const char* id) { EscapableHandleScope scope(isolate()); Local ctx = context(); - MaybeLocal result = BuiltinLoader::CompileAndCall(ctx, id, this); + MaybeLocal result = env()->builtin_loader()->CompileAndCall(ctx, id, this); // If there was an error during bootstrap, it must be unrecoverable // (e.g. max call stack exceeded). Clear the stack so that the diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index efa42b37509e64..21fe26caa1b32b 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1202,10 +1202,10 @@ ExitCode SnapshotBuilder::Generate(SnapshotData* out, #ifdef NODE_USE_NODE_CODE_CACHE // Regenerate all the code cache. - if (!builtins::BuiltinLoader::CompileAllBuiltins(main_context)) { + if (!env->builtin_loader()->CompileAllBuiltins(main_context)) { return ExitCode::kGenericUserError; } - builtins::BuiltinLoader::CopyCodeCache(&(out->code_cache)); + env->builtin_loader()->CopyCodeCache(&(out->code_cache)); for (const auto& item : out->code_cache) { std::string size_str = FormatSize(item.data.size()); per_process::Debug(DebugCategory::MKSNAPSHOT, diff --git a/src/node_worker.cc b/src/node_worker.cc index 3cbede5f3cf7ca..8067c519d8b835 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -349,6 +349,8 @@ void Worker::Run() { static_cast(environment_flags_), thread_id_, std::move(inspector_parent_handle_))); + // TODO(addaleax): Adjust for the embedder API snapshot support changes + env_->set_builtin_loader(env()->builtin_loader()); if (is_stopped()) return; CHECK_NOT_NULL(env_); env_->set_env_vars(std::move(env_vars_)); diff --git a/test/cctest/test_per_process.cc b/test/cctest/test_per_process.cc index 1e3dff7114e337..1cd7adba7bb90f 100644 --- a/test/cctest/test_per_process.cc +++ b/test/cctest/test_per_process.cc @@ -11,7 +11,7 @@ using node::builtins::BuiltinSourceMap; class PerProcessTest : public ::testing::Test { protected: static const BuiltinSourceMap get_sources_for_test() { - return BuiltinLoader::instance_.source_; + return BuiltinLoader::Create()->source_; } };