From be7dcbab7de1178e31e563b34d8eb38d43414ddd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 22 Dec 2022 18:40:01 +0100 Subject: [PATCH 01/11] 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 | 8 +- src/env-inl.h | 10 ++ src/env.cc | 2 + src/env.h | 5 + src/node.cc | 3 - src/node_binding.cc | 4 +- src/node_builtins.cc | 169 +++++++++++++++++--------------- src/node_builtins.h | 79 ++++++++------- src/node_main_instance.cc | 5 + src/node_realm.cc | 3 +- src/node_snapshotable.cc | 4 +- src/node_worker.cc | 2 + test/cctest/test_per_process.cc | 2 +- 13 files changed, 166 insertions(+), 130 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 51e1e33a20b5eb..620f7b3f7c1c0e 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -473,7 +473,7 @@ MaybeLocal LoadEnvironment( return LoadEnvironment( env, [&](const StartExecutionCallbackInfo& info) -> MaybeLocal { 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()); @@ -714,10 +714,12 @@ 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( - context, *module, arraysize(arguments), arguments, nullptr) + if (builtin_loader + ->CompileAndCall( + context, *module, arraysize(arguments), arguments, nullptr) .IsEmpty()) { // Execution failed during context creation. return Nothing(); diff --git a/src/env-inl.h b/src/env-inl.h index 6624b667736ec8..acf48b07bfd7a4 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -436,6 +436,16 @@ 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 e6572100ffcb8e..50949856928b45 100644 --- a/src/env.cc +++ b/src/env.cc @@ -752,6 +752,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 521a63cddc5a36..79d49515c0710f 100644 --- a/src/env.h +++ b/src/env.h @@ -721,6 +721,9 @@ class Environment : public MemoryRetainer { // List of id's that have been destroyed and need the destroy() cb called. inline std::vector* destroy_async_id_list(); + std::shared_ptr builtin_loader(); + void set_builtin_loader(std::shared_ptr loader); + std::unordered_multimap hash_to_module_map; std::unordered_map id_to_module_map; std::unordered_map @@ -1144,6 +1147,8 @@ class Environment : public MemoryRetainer { std::unique_ptr principal_realm_ = nullptr; + std::shared_ptr builtin_loader_; + // Used by allocate_managed_buffer() and release_managed_buffer() to keep // track of the BackingStore for a given pointer. std::unordered_map> 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 dc5ddb7b33f221..243a44994fb300 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -631,13 +631,13 @@ void GetInternalBinding(const FunctionCallbackInfo& 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); diff --git a/src/node_builtins.cc b/src/node_builtins.cc index d6b5114aa3c4aa..fd92742d53807d 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -32,8 +32,6 @@ using v8::String; using v8::Undefined; using v8::Value; -BuiltinLoader BuiltinLoader::instance_; - BuiltinLoader::BuiltinLoader() : config_(GetConfig()), has_code_cache_(false) { LoadJavaScriptSource(); #ifdef NODE_SHARED_BUILTIN_CJS_MODULE_LEXER_LEXER_PATH @@ -54,25 +52,25 @@ BuiltinLoader::BuiltinLoader() : config_(GetConfig()), has_code_cache_(false) { #endif // NODE_SHARED_BUILTIN_UNDICI_UNDICI_PATH } -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(); } @@ -80,10 +78,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_) { @@ -92,11 +91,14 @@ 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 @@ -110,10 +112,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 @@ -143,46 +145,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. @@ -209,10 +205,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,14 +236,31 @@ MaybeLocal BuiltinLoader::LoadBuiltinSource(Isolate* isolate, #endif // NODE_BUILTIN_MODULES_PATH } +namespace { +static Mutex externalized_builtins_mutex; +std::unordered_map externalized_builtin_sources; +} // namespace + 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(); + { + 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(); + } + externalized_builtin_sources[id] = source; + } } Add(id, source); @@ -291,7 +305,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. @@ -357,15 +371,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); @@ -428,9 +435,10 @@ MaybeLocal BuiltinLoader::LookupAndCompile(Local context, }; } - MaybeLocal maybe = GetInstance()->LookupAndCompileInternal( - context, id, ¶meters, &result); + MaybeLocal maybe = + LookupAndCompileInternal(context, id, ¶meters, &result); if (optional_realm != nullptr) { + DCHECK_EQ(this, optional_realm->env()->builtin_loader().get()); RecordResult(id, result, optional_realm); } return maybe; @@ -506,8 +514,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) { @@ -515,7 +522,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", @@ -528,10 +535,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}}); @@ -539,24 +544,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( @@ -568,8 +565,9 @@ 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"); @@ -648,16 +646,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, @@ -675,8 +676,8 @@ void BuiltinLoader::CompileFunction(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); node::Utf8Value id_v(realm->isolate(), args[0].As()); const char* id = *id_v; - MaybeLocal maybe = - GetInstance()->LookupAndCompile(realm->context(), id, realm); + MaybeLocal maybe = realm->env()->builtin_loader()->LookupAndCompile( + realm->context(), id, realm); Local fn; if (maybe.ToLocal(&fn)) { args.GetReturnValue().Set(fn); @@ -684,8 +685,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()}; } void BuiltinLoader::CreatePerIsolateProperties(IsolateData* isolate_data, diff --git a/src/node_builtins.h b/src/node_builtins.h index f90a4151850d54..07aae5d697d78d 100644 --- a/src/node_builtins.h +++ b/src/node_builtins.h @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -50,62 +51,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::Local context, const char* id, Realm* optional_realm); + v8::MaybeLocal LookupAndCompile(v8::Local context, + const char* id, + Realm* optional_realm); - static v8::MaybeLocal CompileAndCall( - v8::Local context, - const char* id, - int argc, - v8::Local argv[], - Realm* optional_realm); + v8::MaybeLocal CompileAndCall(v8::Local context, + const char* id, + int argc, + v8::Local argv[], + Realm* optional_realm); - static v8::MaybeLocal CompileAndCall( - v8::Local context, const char* id, Realm* realm); + 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); - static bool Add(const char* id, std::string_view utf8source); + v8::Local GetConfigString(v8::Isolate* isolate); + bool Exists(const char* id); + bool Add(const char* id, const UnionBytes& source); + bool Add(const char* id, std::string_view utf8source); + + bool CompileAllBuiltins(v8::Local context); + void RefreshCodeCache(const std::vector& in); + void CopyCodeCache(std::vector* out); - static bool CompileAllBuiltins(v8::Local context); - static void RefreshCodeCache(const std::vector& in); - static 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(); + const BuiltinCategories& InitializeBuiltinCategories() const; + const std::set& GetCannotBeRequired() const; + const std::set& GetCanBeRequired() const; - bool CanBeRequired(const char* id); - bool CannotBeRequired(const char* id); - - 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( @@ -134,17 +133,23 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { static void HasCachedBuiltins( const v8::FunctionCallbackInfo& args); - static void AddExternalizedBuiltin(const char* id, const char* filename); + void AddExternalizedBuiltin(const char* id, const char* filename); - static BuiltinLoader instance_; - BuiltinCategories builtin_categories_; - BuiltinSourceMap source_; - BuiltinCodeCacheMap code_cache_; - UnionBytes config_; + // 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 - Mutex code_cache_mutex_; + RwLock source_mutex_; + BuiltinSourceMap source_; + + 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 c9126fb0cce220..5a0497ccf41c3b 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -159,6 +159,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 f1c02bdd61855d..a12973e20ecf10 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -174,7 +174,8 @@ 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 7c64006efb068b..f74bbc0bc41ae9 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -1205,10 +1205,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 b84a39cce330d7..4c3053c32fbeb8 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -344,6 +344,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_; } }; From 2d3190ce31106d7e957f89b89c2f320dba2d49cd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 6 Jan 2023 18:51:13 +0100 Subject: [PATCH 02/11] fixup! src: make BuiltinLoader threadsafe and non-global --- src/env-inl.h | 9 +-- src/env.cc | 5 +- src/env.h | 5 +- src/node_builtins.cc | 53 ++++++++++------- src/node_builtins.h | 15 ++--- src/node_main_instance.cc | 6 +- src/node_threadsafe_cow-inl.h | 54 +++++++++++++++++ src/node_threadsafe_cow.h | 102 ++++++++++++++++++++++++++++++++ src/node_worker.cc | 5 +- test/cctest/test_per_process.cc | 2 +- tools/js2c.py | 10 +++- 11 files changed, 216 insertions(+), 50 deletions(-) create mode 100644 src/node_threadsafe_cow-inl.h create mode 100644 src/node_threadsafe_cow.h diff --git a/src/env-inl.h b/src/env-inl.h index acf48b07bfd7a4..40cf3c901d368f 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -436,14 +436,9 @@ inline std::vector* Environment::destroy_async_id_list() { return &destroy_async_id_list_; } -inline std::shared_ptr Environment::builtin_loader() { +inline builtins::BuiltinLoader* Environment::builtin_loader() { DCHECK(builtin_loader_); - return builtin_loader_; -} - -inline void Environment::set_builtin_loader( - std::shared_ptr loader) { - builtin_loader_ = loader; + return builtin_loader_.get(); } inline double Environment::new_async_id() { diff --git a/src/env.cc b/src/env.cc index 50949856928b45..43170273aab30b 100644 --- a/src/env.cc +++ b/src/env.cc @@ -674,7 +674,8 @@ Environment::Environment(IsolateData* isolate_data, flags_(flags), thread_id_(thread_id.id == static_cast(-1) ? AllocateEnvironmentThreadId().id - : thread_id.id) { + : thread_id.id), + builtin_loader_(builtins::BuiltinLoader::Create()) { // We'll be creating new objects so make sure we've entered the context. HandleScope handle_scope(isolate); @@ -752,8 +753,6 @@ 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 79d49515c0710f..ca25b309ea75f0 100644 --- a/src/env.h +++ b/src/env.h @@ -721,8 +721,7 @@ class Environment : public MemoryRetainer { // List of id's that have been destroyed and need the destroy() cb called. inline std::vector* destroy_async_id_list(); - std::shared_ptr builtin_loader(); - void set_builtin_loader(std::shared_ptr loader); + builtins::BuiltinLoader* builtin_loader(); std::unordered_multimap hash_to_module_map; std::unordered_map id_to_module_map; @@ -1147,7 +1146,7 @@ class Environment : public MemoryRetainer { std::unique_ptr principal_realm_ = nullptr; - std::shared_ptr builtin_loader_; + std::unique_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_builtins.cc b/src/node_builtins.cc index fd92742d53807d..ad896d6b6f2b0c 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -3,6 +3,7 @@ #include "env-inl.h" #include "node_external_reference.h" #include "node_internals.h" +#include "node_threadsafe_cow-inl.h" #include "simdutf.h" #include "util-inl.h" @@ -53,24 +54,22 @@ BuiltinLoader::BuiltinLoader() : config_(GetConfig()), has_code_cache_(false) { } bool BuiltinLoader::Exists(const char* id) { - RwLock::ScopedReadLock lock(source_mutex_); - return source_.find(id) != source_.end(); + auto source = source_.read(); + return source->find(id) != source->end(); } bool BuiltinLoader::Add(const char* id, const UnionBytes& 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); + auto result = source_.write()->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); - for (auto const& x : source_) { + auto source = source_.read(); + 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(); } @@ -82,10 +81,10 @@ Local BuiltinLoader::GetConfigString(Isolate* isolate) { } std::vector BuiltinLoader::GetBuiltinIds() const { - RwLock::ScopedReadLock lock(source_mutex_); std::vector ids; - ids.reserve(source_.size()); - for (auto const& x : source_) { + auto source = source_.read(); + ids.reserve(source->size()); + for (auto const& x : *source) { ids.emplace_back(x.first); } return ids; @@ -93,7 +92,6 @@ std::vector BuiltinLoader::GetBuiltinIds() const { 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(); @@ -138,7 +136,8 @@ BuiltinLoader::InitializeBuiltinCategories() const { "internal/v8_prof_processor", }; - for (auto const& x : source_) { + auto source = source_.read(); + for (auto const& x : *source) { const std::string& id = x.first; for (auto const& prefix : prefixes) { if (prefix.length() > id.length()) { @@ -151,7 +150,7 @@ BuiltinLoader::InitializeBuiltinCategories() const { } } - for (auto const& x : source_) { + 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); @@ -177,7 +176,8 @@ bool BuiltinLoader::CannotBeRequired(const char* id) const { return GetCannotBeRequired().count(id) == 1; } -ScriptCompiler::CachedData* BuiltinLoader::GetCodeCache(const char* id) const { +const ScriptCompiler::CachedData* BuiltinLoader::GetCodeCache( + const char* id) const { RwLock::ScopedReadLock lock(code_cache_mutex_); const auto it = code_cache_.find(id); if (it == code_cache_.end()) { @@ -206,12 +206,12 @@ static std::string OnDiskFileName(const char* id) { MaybeLocal BuiltinLoader::LoadBuiltinSource(Isolate* isolate, const char* id) const { + auto source = source_.read(); #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())) { + const auto source_it = source->find(id); + if (UNLIKELY(source_it == source->end())) { fprintf(stderr, "Cannot find native builtin: \"%s\".\n", id); ABORT(); } @@ -438,7 +438,7 @@ MaybeLocal BuiltinLoader::LookupAndCompile(Local context, MaybeLocal maybe = LookupAndCompileInternal(context, id, ¶meters, &result); if (optional_realm != nullptr) { - DCHECK_EQ(this, optional_realm->env()->builtin_loader().get()); + DCHECK_EQ(this, optional_realm->env()->builtin_loader()); RecordResult(id, result, optional_realm); } return maybe; @@ -534,7 +534,7 @@ bool BuiltinLoader::CompileAllBuiltins(Local context) { return all_succeeded; } -void BuiltinLoader::CopyCodeCache(std::vector* out) { +void BuiltinLoader::CopyCodeCache(std::vector* out) const { RwLock::ScopedReadLock lock(code_cache_mutex_); for (auto const& item : code_cache_) { out->push_back( @@ -691,8 +691,19 @@ void BuiltinLoader::HasCachedBuiltins(const FunctionCallbackInfo& args) { v8::Boolean::New(args.GetIsolate(), instance->has_code_cache_)); } -std::shared_ptr BuiltinLoader::Create() { - return std::shared_ptr{new BuiltinLoader()}; +std::unique_ptr BuiltinLoader::Create() { + return std::unique_ptr{new BuiltinLoader()}; +} + +void BuiltinLoader::CopySourceAndCodeCacheFrom(const BuiltinLoader* other) { + { + std::vector cache; + other->CopyCodeCache(&cache); + RefreshCodeCache(cache); + has_code_cache_ = other->has_code_cache_; + } + + source_ = other->source_; } void BuiltinLoader::CreatePerIsolateProperties(IsolateData* isolate_data, diff --git a/src/node_builtins.h b/src/node_builtins.h index 07aae5d697d78d..dad3c0c4d3ae06 100644 --- a/src/node_builtins.h +++ b/src/node_builtins.h @@ -11,6 +11,7 @@ #include #include #include "node_mutex.h" +#include "node_threadsafe_cow.h" #include "node_union_bytes.h" #include "v8.h" @@ -74,9 +75,10 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { bool CompileAllBuiltins(v8::Local context); void RefreshCodeCache(const std::vector& in); - void CopyCodeCache(std::vector* out); + void CopyCodeCache(std::vector* out) const; - static std::shared_ptr Create(); + static std::unique_ptr Create(); + void CopySourceAndCodeCacheFrom(const BuiltinLoader* other); private: // Only allow access from friends. @@ -101,7 +103,7 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { bool CanBeRequired(const char* id) const; bool CannotBeRequired(const char* id) const; - v8::ScriptCompiler::CachedData* GetCodeCache(const char* id) const; + const v8::ScriptCompiler::CachedData* GetCodeCache(const char* id) const; enum class Result { kWithCache, kWithoutCache }; v8::MaybeLocal LoadBuiltinSource(v8::Isolate* isolate, const char* id) const; @@ -135,19 +137,14 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { void AddExternalizedBuiltin(const char* id, const char* filename); - // 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_; + ThreadsafeCopyOnWrite source_; const UnionBytes config_; - // Used to synchronize access to the code cache map RwLock code_cache_mutex_; BuiltinCodeCacheMap code_cache_; bool has_code_cache_; diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc index 5a0497ccf41c3b..a40866b482c326 100644 --- a/src/node_main_instance.cc +++ b/src/node_main_instance.cc @@ -159,11 +159,11 @@ NodeMainInstance::CreateMainEnvironment(ExitCode* exit_code) { &(snapshot_data_->env_info), EnvironmentFlags::kDefaultFlags, {})); +#ifdef NODE_V8_SHARED_RO_HEAP // 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); + env->builtin_loader()->RefreshCodeCache(snapshot_data_->code_cache); +#endif context = Context::FromSnapshot(isolate_, SnapshotData::kNodeMainContextIndex, {DeserializeNodeInternalFields, env.get()}) diff --git a/src/node_threadsafe_cow-inl.h b/src/node_threadsafe_cow-inl.h new file mode 100644 index 00000000000000..875b38766063f6 --- /dev/null +++ b/src/node_threadsafe_cow-inl.h @@ -0,0 +1,54 @@ +#ifndef SRC_NODE_THREADSAFE_COW_INL_H_ +#define SRC_NODE_THREADSAFE_COW_INL_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +namespace node { + +template +T* CopyOnWrite::write() { + if (!data_.unique()) { + data_ = std::make_shared(*data_); + } + return data_.get(); +} + +template +ThreadsafeCopyOnWrite::Read::Read(const ThreadsafeCopyOnWrite* cow) + : cow_(cow), lock_(cow->impl_->mutex) {} + +template +const T& ThreadsafeCopyOnWrite::Read::operator*() const { + return cow_->impl_->data; +} + +template +const T* ThreadsafeCopyOnWrite::Read::operator->() const { + return &cow_->impl_->data; +} + +template +ThreadsafeCopyOnWrite::Write::Write(ThreadsafeCopyOnWrite* cow) + : cow_(cow), impl_(cow->impl_.write()), lock_(impl_->mutex) {} + +template +T& ThreadsafeCopyOnWrite::Write::operator*() { + return impl_->data; +} + +template +T* ThreadsafeCopyOnWrite::Write::operator->() { + return &impl_->data; +} + +template +ThreadsafeCopyOnWrite::Impl::Impl(const Impl& other) { + RwLock::ScopedReadLock lock(other.mutex); + data = other.data; +} + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_NODE_THREADSAFE_COW_INL_H_ diff --git a/src/node_threadsafe_cow.h b/src/node_threadsafe_cow.h new file mode 100644 index 00000000000000..326e967adcd059 --- /dev/null +++ b/src/node_threadsafe_cow.h @@ -0,0 +1,102 @@ +#ifndef SRC_NODE_THREADSAFE_COW_H_ +#define SRC_NODE_THREADSAFE_COW_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "util.h" +#include "uv.h" + +#include // std::shared_ptr +#include // std::forward + +namespace node { + +// Copy-on-write utility. Not threadsafe, i.e. there is no synchronization +// of the copy operation with other operations. +template +class CopyOnWrite final { + public: + template + explicit CopyOnWrite(Args&&... args) + : data_(std::make_shared(std::forward(args)...)) {} + + CopyOnWrite(const CopyOnWrite& other) = default; + CopyOnWrite& operator=(const CopyOnWrite& other) = default; + CopyOnWrite(CopyOnWrite&& other) = default; + CopyOnWrite& operator=(CopyOnWrite&& other) = default; + + const T* read() const { return data_.get(); } + T* write(); + + const T& operator*() const { return *read(); } + const T* operator->() const { return read(); } + + private: + std::shared_ptr data_; +}; + +// Threadsafe copy-on-write utility. Consumers need to use the Read and +// Write helpers to access the target data structure. +template +class ThreadsafeCopyOnWrite final { + public: + template + ThreadsafeCopyOnWrite(Args&&... args) + : impl_(T(std::forward(args)...)) {} + + ThreadsafeCopyOnWrite(const ThreadsafeCopyOnWrite& other) = default; + ThreadsafeCopyOnWrite& operator=(const ThreadsafeCopyOnWrite& other) = + default; + ThreadsafeCopyOnWrite(ThreadsafeCopyOnWrite&& other) = default; + ThreadsafeCopyOnWrite& operator=(ThreadsafeCopyOnWrite&& other) = default; + + class Read { + public: + explicit Read(const ThreadsafeCopyOnWrite* cow); + + const T& operator*() const; + const T* operator->() const; + + private: + const ThreadsafeCopyOnWrite* cow_; + RwLock::ScopedReadLock lock_; + }; + + class Write { + public: + explicit Write(ThreadsafeCopyOnWrite* cow); + + T& operator*(); + T* operator->(); + + private: + ThreadsafeCopyOnWrite* cow_; + typename ThreadsafeCopyOnWrite::Impl* impl_; + RwLock::ScopedLock lock_; + }; + + Read read() const { return Read(this); } + Write write() { return Write(this); } + + private: + struct Impl { + Impl(const T& data) : data(data) {} + Impl(T&& data) : data(std::move(data)) {} + + Impl(const Impl& other); + Impl& operator=(const Impl& other) = delete; + Impl(Impl&& other) = delete; + Impl& operator=(Impl&& other) = delete; + + RwLock mutex; + T data; + }; + + CopyOnWrite impl_; +}; + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_NODE_THREADSAFE_COW_H_ diff --git a/src/node_worker.cc b/src/node_worker.cc index 4c3053c32fbeb8..fb30046337cd64 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -344,8 +344,11 @@ void Worker::Run() { static_cast(environment_flags_), thread_id_, std::move(inspector_parent_handle_))); +#ifdef NODE_V8_SHARED_RO_HEAP // TODO(addaleax): Adjust for the embedder API snapshot support changes - env_->set_builtin_loader(env()->builtin_loader()); + env_->builtin_loader()->CopySourceAndCodeCacheFrom( + env()->builtin_loader()); +#endif 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 1cd7adba7bb90f..93904f5c5b2120 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::Create()->source_; + return *BuiltinLoader::Create()->source_.read(); } }; diff --git a/tools/js2c.py b/tools/js2c.py index e295949a18508d..b8b26efc882a6d 100755 --- a/tools/js2c.py +++ b/tools/js2c.py @@ -57,8 +57,14 @@ def ReadFile(filename): {0} +namespace {{ +const ThreadsafeCopyOnWrite global_source_map {{ + BuiltinSourceMap{{ {1} }} +}}; +}} + void BuiltinLoader::LoadJavaScriptSource() {{ - {1} + source_ = global_source_map; }} UnionBytes BuiltinLoader::GetConfig() {{ @@ -82,7 +88,7 @@ def ReadFile(filename): }}; """ -INITIALIZER = 'source_.emplace("{0}", UnionBytes{{{1}, {2}}});' +INITIALIZER = '{{"{0}", UnionBytes{{{1}, {2}}} }},' CONFIG_GYPI_ID = 'config_raw' From 1bfd5acc4af1b374db6792a65a5808aeb76e345e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 6 Jan 2023 19:03:06 +0100 Subject: [PATCH 03/11] fixup! fixup! src: make BuiltinLoader threadsafe and non-global --- src/node.cc | 2 -- src/node_realm.cc | 1 - src/node_threadsafe_cow.h | 4 ++-- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/node.cc b/src/node.cc index e77265a46cb2ea..c5b6c2b8444499 100644 --- a/src/node.cc +++ b/src/node.cc @@ -126,8 +126,6 @@ namespace node { -using builtins::BuiltinLoader; - using v8::EscapableHandleScope; using v8::Isolate; using v8::Local; diff --git a/src/node_realm.cc b/src/node_realm.cc index a12973e20ecf10..e0fcd3a0775973 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -8,7 +8,6 @@ namespace node { -using builtins::BuiltinLoader; using v8::Context; using v8::EscapableHandleScope; using v8::Function; diff --git a/src/node_threadsafe_cow.h b/src/node_threadsafe_cow.h index 326e967adcd059..00f1f923ea20b4 100644 --- a/src/node_threadsafe_cow.h +++ b/src/node_threadsafe_cow.h @@ -80,8 +80,8 @@ class ThreadsafeCopyOnWrite final { private: struct Impl { - Impl(const T& data) : data(data) {} - Impl(T&& data) : data(std::move(data)) {} + explicit Impl(const T& data) : data(data) {} + explicit Impl(T&& data) : data(std::move(data)) {} Impl(const Impl& other); Impl& operator=(const Impl& other) = delete; From d015622d28452a6ad4e3f9c5b29c3bc3c1b0f4d6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 10 Jan 2023 14:07:20 +0100 Subject: [PATCH 04/11] fixup! fixup! fixup! src: make BuiltinLoader threadsafe and non-global --- test/cctest/test_per_process.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/cctest/test_per_process.cc b/test/cctest/test_per_process.cc index 93904f5c5b2120..710e863f457e9c 100644 --- a/test/cctest/test_per_process.cc +++ b/test/cctest/test_per_process.cc @@ -1,4 +1,5 @@ #include "node_builtins.h" +#include "node_threadsafe_cow-inl.h" #include "gtest/gtest.h" #include "node_test_fixture.h" From 93e411acab1a6a21b6378ee0f56579a4434bd1ea Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 11 Jan 2023 12:34:19 +0100 Subject: [PATCH 05/11] fixup! fixup! fixup! fixup! src: make BuiltinLoader threadsafe and non-global --- src/node_threadsafe_cow.h | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/node_threadsafe_cow.h b/src/node_threadsafe_cow.h index 00f1f923ea20b4..8cfdd006b12f57 100644 --- a/src/node_threadsafe_cow.h +++ b/src/node_threadsafe_cow.h @@ -39,6 +39,22 @@ class CopyOnWrite final { // Write helpers to access the target data structure. template class ThreadsafeCopyOnWrite final { + private: + // Define this early since some of the public members depend on it + // and some compilers need it to be defined first in that case. + struct Impl { + explicit Impl(const T& data) : data(data) {} + explicit Impl(T&& data) : data(std::move(data)) {} + + Impl(const Impl& other); + Impl& operator=(const Impl& other) = delete; + Impl(Impl&& other) = delete; + Impl& operator=(Impl&& other) = delete; + + RwLock mutex; + T data; + }; + public: template ThreadsafeCopyOnWrite(Args&&... args) @@ -79,19 +95,6 @@ class ThreadsafeCopyOnWrite final { Write write() { return Write(this); } private: - struct Impl { - explicit Impl(const T& data) : data(data) {} - explicit Impl(T&& data) : data(std::move(data)) {} - - Impl(const Impl& other); - Impl& operator=(const Impl& other) = delete; - Impl(Impl&& other) = delete; - Impl& operator=(Impl&& other) = delete; - - RwLock mutex; - T data; - }; - CopyOnWrite impl_; }; From ed986f9a84ea04b70abf741d59cd3a0f0759b60c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 17 Jan 2023 15:05:32 +0100 Subject: [PATCH 06/11] fixup! fixup! fixup! fixup! fixup! src: make BuiltinLoader threadsafe and non-global --- src/api/environment.cc | 7 ++++++- src/node_builtins.cc | 1 - 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 620f7b3f7c1c0e..ab8ef2f2d838ee 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -714,7 +714,12 @@ Maybe InitializePrimordials(Local context) { "internal/per_context/messageport", nullptr}; - auto builtin_loader = builtins::BuiltinLoader::Create(); + // 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 auto builtin_loader = builtins::BuiltinLoader::Create(); for (const char** module = context_files; *module != nullptr; module++) { Local arguments[] = {exports, primordials}; if (builtin_loader diff --git a/src/node_builtins.cc b/src/node_builtins.cc index ad896d6b6f2b0c..77fffd81b696ba 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -563,7 +563,6 @@ void BuiltinLoader::GetBuiltinCategories( Local context = env->context(); Local result = Object::New(isolate); - // Copy from the per-process categories std::set cannot_be_required = env->builtin_loader()->GetCannotBeRequired(); std::set can_be_required = From fcdb475f4927a6cebedfae4da795b7af08c9f6ad Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 17 Jan 2023 17:58:18 +0100 Subject: [PATCH 07/11] fixup! fixup! fixup! fixup! fixup! fixup! src: make BuiltinLoader threadsafe and non-global --- src/node_builtins.cc | 44 +++++++++++--------------------------------- src/node_builtins.h | 14 ++++---------- 2 files changed, 15 insertions(+), 43 deletions(-) diff --git a/src/node_builtins.cc b/src/node_builtins.cc index 77fffd81b696ba..bc1e9e41b0b557 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -59,8 +59,6 @@ bool BuiltinLoader::Exists(const char* id) { } bool BuiltinLoader::Add(const char* id, const UnionBytes& source) { - builtin_categories_ - .reset(); // The category cache is reset by adding new sources auto result = source_.write()->emplace(id, source); return result.second; } @@ -90,13 +88,8 @@ std::vector BuiltinLoader::GetBuiltinIds() const { return ids; } -const BuiltinLoader::BuiltinCategories& -BuiltinLoader::InitializeBuiltinCategories() const { - if (LIKELY(builtin_categories_.has_value())) { - DCHECK(!builtin_categories_.value().can_be_required.empty()); - return builtin_categories_.value(); - } - auto& builtin_categories = builtin_categories_.emplace(); +BuiltinLoader::BuiltinCategories BuiltinLoader::GetBuiltinCategories() const { + BuiltinCategories builtin_categories; std::vector prefixes = { #if !HAVE_OPENSSL @@ -160,22 +153,6 @@ BuiltinLoader::InitializeBuiltinCategories() const { return builtin_categories; } -const std::set& BuiltinLoader::GetCannotBeRequired() const { - return InitializeBuiltinCategories().cannot_be_required; -} - -const std::set& BuiltinLoader::GetCanBeRequired() const { - return InitializeBuiltinCategories().can_be_required; -} - -bool BuiltinLoader::CanBeRequired(const char* id) const { - return GetCanBeRequired().count(id) == 1; -} - -bool BuiltinLoader::CannotBeRequired(const char* id) const { - return GetCannotBeRequired().count(id) == 1; -} - const ScriptCompiler::CachedData* BuiltinLoader::GetCodeCache( const char* id) const { RwLock::ScopedReadLock lock(code_cache_mutex_); @@ -563,20 +540,19 @@ void BuiltinLoader::GetBuiltinCategories( Local context = env->context(); Local result = Object::New(isolate); - std::set cannot_be_required = - env->builtin_loader()->GetCannotBeRequired(); - std::set can_be_required = - env->builtin_loader()->GetCanBeRequired(); + BuiltinCategories builtin_categories = + env->builtin_loader()->GetBuiltinCategories(); if (!env->owns_process_state()) { - can_be_required.erase("trace_events"); - cannot_be_required.insert("trace_events"); + builtin_categories.can_be_required.erase("trace_events"); + builtin_categories.cannot_be_required.insert("trace_events"); } Local cannot_be_required_js; Local can_be_required_js; - if (!ToV8Value(context, cannot_be_required).ToLocal(&cannot_be_required_js)) + if (!ToV8Value(context, builtin_categories.cannot_be_required) + .ToLocal(&cannot_be_required_js)) return; if (result ->Set(context, @@ -584,7 +560,9 @@ void BuiltinLoader::GetBuiltinCategories( cannot_be_required_js) .IsNothing()) return; - if (!ToV8Value(context, can_be_required).ToLocal(&can_be_required_js)) return; + if (!ToV8Value(context, builtin_categories.can_be_required) + .ToLocal(&can_be_required_js)) + return; if (result ->Set(context, OneByteString(isolate, "canBeRequired"), diff --git a/src/node_builtins.h b/src/node_builtins.h index dad3c0c4d3ae06..35eeac1ac9b7e4 100644 --- a/src/node_builtins.h +++ b/src/node_builtins.h @@ -96,12 +96,10 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { std::set can_be_required; std::set cannot_be_required; }; - const BuiltinCategories& InitializeBuiltinCategories() const; - const std::set& GetCannotBeRequired() const; - const std::set& GetCanBeRequired() const; - - bool CanBeRequired(const char* id) const; - bool CannotBeRequired(const char* id) const; + // This method builds `BuiltinCategories` from scratch every time, + // and is therefore somewhat expensive, but also currently only being + // used for testing, so that should not be an issue. + BuiltinCategories GetBuiltinCategories() const; const v8::ScriptCompiler::CachedData* GetCodeCache(const char* id) const; enum class Result { kWithCache, kWithoutCache }; @@ -137,10 +135,6 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { void AddExternalizedBuiltin(const char* id, const char* filename); - // builtin_categories is marked mutable because it is only initialized - // as a cache, so that mutating it does not change any state. - mutable std::optional builtin_categories_; - ThreadsafeCopyOnWrite source_; const UnionBytes config_; From 278a5705e69f9b6501f927ab5a8ac34cfa0ed4c7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 18 Jan 2023 18:22:05 +0100 Subject: [PATCH 08/11] fixup! fixup! fixup! fixup! fixup! fixup! fixup! src: make BuiltinLoader threadsafe and non-global --- src/env.cc | 8 ++++++++ src/node_worker.cc | 5 ----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/env.cc b/src/env.cc index 43170273aab30b..db1c39426ed9f5 100644 --- a/src/env.cc +++ b/src/env.cc @@ -676,6 +676,14 @@ Environment::Environment(IsolateData* isolate_data, ? AllocateEnvironmentThreadId().id : thread_id.id), builtin_loader_(builtins::BuiltinLoader::Create()) { +#ifdef NODE_V8_SHARED_RO_HEAP + if (isolate_data->worker_context() != nullptr) { + // TODO(addaleax): Adjust for the embedder API snapshot support changes + builtin_loader()->CopySourceAndCodeCacheFrom( + 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 --git a/src/node_worker.cc b/src/node_worker.cc index fb30046337cd64..b84a39cce330d7 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -344,11 +344,6 @@ void Worker::Run() { static_cast(environment_flags_), thread_id_, std::move(inspector_parent_handle_))); -#ifdef NODE_V8_SHARED_RO_HEAP - // TODO(addaleax): Adjust for the embedder API snapshot support changes - env_->builtin_loader()->CopySourceAndCodeCacheFrom( - env()->builtin_loader()); -#endif if (is_stopped()) return; CHECK_NOT_NULL(env_); env_->set_env_vars(std::move(env_vars_)); From cedd64a182091656cebcbb3d11c756b3a4050007 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 18 Jan 2023 18:33:45 +0100 Subject: [PATCH 09/11] fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! src: make BuiltinLoader threadsafe and non-global --- src/api/environment.cc | 4 ++-- src/env-inl.h | 3 +-- src/env.cc | 3 +-- src/env.h | 2 +- src/node_builtins.h | 3 +-- test/cctest/test_per_process.cc | 2 +- 6 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index ab8ef2f2d838ee..406455c7263573 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -719,11 +719,11 @@ Maybe InitializePrimordials(Local context) { // 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 auto builtin_loader = builtins::BuiltinLoader::Create(); + thread_local builtins::BuiltinLoader builtin_loader; for (const char** module = context_files; *module != nullptr; module++) { Local arguments[] = {exports, primordials}; if (builtin_loader - ->CompileAndCall( + .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 40cf3c901d368f..4dd4ee5e1d7c9e 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -437,8 +437,7 @@ inline std::vector* Environment::destroy_async_id_list() { } inline builtins::BuiltinLoader* Environment::builtin_loader() { - DCHECK(builtin_loader_); - return builtin_loader_.get(); + return &builtin_loader_; } inline double Environment::new_async_id() { diff --git a/src/env.cc b/src/env.cc index db1c39426ed9f5..56d488a3ef7080 100644 --- a/src/env.cc +++ b/src/env.cc @@ -674,8 +674,7 @@ Environment::Environment(IsolateData* isolate_data, flags_(flags), thread_id_(thread_id.id == static_cast(-1) ? AllocateEnvironmentThreadId().id - : thread_id.id), - builtin_loader_(builtins::BuiltinLoader::Create()) { + : thread_id.id) { #ifdef NODE_V8_SHARED_RO_HEAP if (isolate_data->worker_context() != nullptr) { // TODO(addaleax): Adjust for the embedder API snapshot support changes diff --git a/src/env.h b/src/env.h index ca25b309ea75f0..2619e57e44d6b9 100644 --- a/src/env.h +++ b/src/env.h @@ -1146,7 +1146,7 @@ class Environment : public MemoryRetainer { std::unique_ptr principal_realm_ = nullptr; - std::unique_ptr builtin_loader_; + builtins::BuiltinLoader 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_builtins.h b/src/node_builtins.h index 35eeac1ac9b7e4..20a6405e0f0aa7 100644 --- a/src/node_builtins.h +++ b/src/node_builtins.h @@ -39,6 +39,7 @@ struct CodeCacheInfo { // bootstrap scripts, whose source are bundled into the binary as static data. class NODE_EXTERN_PRIVATE BuiltinLoader { public: + BuiltinLoader(); BuiltinLoader(const BuiltinLoader&) = delete; BuiltinLoader& operator=(const BuiltinLoader&) = delete; @@ -84,8 +85,6 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { // Only allow access from friends. friend class CodeCacheBuilder; - BuiltinLoader(); - // Generated by tools/js2c.py as node_javascript.cc void LoadJavaScriptSource(); // Loads data into source_ UnionBytes GetConfig(); // Return data for config.gypi diff --git a/test/cctest/test_per_process.cc b/test/cctest/test_per_process.cc index 710e863f457e9c..34cf163add7904 100644 --- a/test/cctest/test_per_process.cc +++ b/test/cctest/test_per_process.cc @@ -12,7 +12,7 @@ using node::builtins::BuiltinSourceMap; class PerProcessTest : public ::testing::Test { protected: static const BuiltinSourceMap get_sources_for_test() { - return *BuiltinLoader::Create()->source_.read(); + return *BuiltinLoader().source_.read(); } }; From 2c2084c07fc8e6ecf559ff5f2b7dd880ef3a799d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 18 Jan 2023 19:01:33 +0100 Subject: [PATCH 10/11] fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! src: make BuiltinLoader threadsafe and non-global --- src/env.cc | 2 +- src/node_builtins.cc | 48 ++++++++++++++++++++------------------------ src/node_builtins.h | 11 ++++++---- 3 files changed, 30 insertions(+), 31 deletions(-) diff --git a/src/env.cc b/src/env.cc index 56d488a3ef7080..62d834a9659972 100644 --- a/src/env.cc +++ b/src/env.cc @@ -678,7 +678,7 @@ Environment::Environment(IsolateData* isolate_data, #ifdef NODE_V8_SHARED_RO_HEAP if (isolate_data->worker_context() != nullptr) { // TODO(addaleax): Adjust for the embedder API snapshot support changes - builtin_loader()->CopySourceAndCodeCacheFrom( + builtin_loader()->CopySourceAndCodeCacheReferenceFrom( isolate_data->worker_context()->env()->builtin_loader()); } #endif diff --git a/src/node_builtins.cc b/src/node_builtins.cc index bc1e9e41b0b557..2b9899c3c2251b 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -33,7 +33,8 @@ using v8::String; using v8::Undefined; using v8::Value; -BuiltinLoader::BuiltinLoader() : config_(GetConfig()), has_code_cache_(false) { +BuiltinLoader::BuiltinLoader() + : config_(GetConfig()), code_cache_(std::make_shared()) { LoadJavaScriptSource(); #ifdef NODE_SHARED_BUILTIN_CJS_MODULE_LEXER_LEXER_PATH AddExternalizedBuiltin( @@ -155,9 +156,9 @@ BuiltinLoader::BuiltinCategories BuiltinLoader::GetBuiltinCategories() const { const ScriptCompiler::CachedData* BuiltinLoader::GetCodeCache( const char* id) const { - RwLock::ScopedReadLock lock(code_cache_mutex_); - const auto it = code_cache_.find(id); - if (it == code_cache_.end()) { + RwLock::ScopedReadLock lock(code_cache_->mutex); + const auto it = code_cache_->map.find(id); + if (it == code_cache_->map.end()) { // The module has not been compiled before. return nullptr; } @@ -282,12 +283,12 @@ 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). - RwLock::ScopedLock lock(code_cache_mutex_); - auto cache_it = code_cache_.find(id); - if (cache_it != code_cache_.end()) { + RwLock::ScopedLock lock(code_cache_->mutex); + auto cache_it = code_cache_->map.find(id); + if (cache_it != code_cache_->map.end()) { // Transfer ownership to ScriptCompiler::Source later. cached_data = cache_it->second.release(); - code_cache_.erase(cache_it); + code_cache_->map.erase(cache_it); } } @@ -348,8 +349,8 @@ MaybeLocal BuiltinLoader::LookupAndCompileInternal( CHECK_NOT_NULL(new_cached_data); { - RwLock::ScopedLock lock(code_cache_mutex_); - code_cache_[id] = std::move(new_cached_data); + RwLock::ScopedLock lock(code_cache_->mutex); + code_cache_->map[id] = std::move(new_cached_data); } return scope.Escape(fun); @@ -512,8 +513,8 @@ bool BuiltinLoader::CompileAllBuiltins(Local context) { } void BuiltinLoader::CopyCodeCache(std::vector* out) const { - RwLock::ScopedReadLock lock(code_cache_mutex_); - for (auto const& item : code_cache_) { + RwLock::ScopedReadLock lock(code_cache_->mutex); + for (auto const& item : code_cache_->map) { out->push_back( {item.first, {item.second->data, item.second->data + item.second->length}}); @@ -521,16 +522,16 @@ void BuiltinLoader::CopyCodeCache(std::vector* out) const { } void BuiltinLoader::RefreshCodeCache(const std::vector& in) { - RwLock::ScopedLock lock(code_cache_mutex_); + 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); - code_cache_[item.id] = std::move(new_cache); + code_cache_->map[item.id] = std::move(new_cache); } - has_code_cache_ = true; + code_cache_->has_code_cache = true; } void BuiltinLoader::GetBuiltinCategories( @@ -663,23 +664,18 @@ 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(), instance->has_code_cache_)); + RwLock::ScopedReadLock lock(instance->code_cache_->mutex); + args.GetReturnValue().Set(v8::Boolean::New( + args.GetIsolate(), instance->code_cache_->has_code_cache)); } std::unique_ptr BuiltinLoader::Create() { return std::unique_ptr{new BuiltinLoader()}; } -void BuiltinLoader::CopySourceAndCodeCacheFrom(const BuiltinLoader* other) { - { - std::vector cache; - other->CopyCodeCache(&cache); - RefreshCodeCache(cache); - has_code_cache_ = other->has_code_cache_; - } - +void BuiltinLoader::CopySourceAndCodeCacheReferenceFrom( + const BuiltinLoader* other) { + code_cache_ = other->code_cache_; source_ = other->source_; } diff --git a/src/node_builtins.h b/src/node_builtins.h index 20a6405e0f0aa7..53c323455fca72 100644 --- a/src/node_builtins.h +++ b/src/node_builtins.h @@ -79,7 +79,7 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { void CopyCodeCache(std::vector* out) const; static std::unique_ptr Create(); - void CopySourceAndCodeCacheFrom(const BuiltinLoader* other); + void CopySourceAndCodeCacheReferenceFrom(const BuiltinLoader* other); private: // Only allow access from friends. @@ -138,9 +138,12 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { const UnionBytes config_; - RwLock code_cache_mutex_; - BuiltinCodeCacheMap code_cache_; - bool has_code_cache_; + struct BuiltinCodeCache { + RwLock mutex; + BuiltinCodeCacheMap map; + bool has_code_cache = false; + }; + std::shared_ptr code_cache_; friend class ::PerProcessTest; }; From 492ede92978f51410022463795f3dd37a0c6ea19 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 18 Jan 2023 19:49:56 +0100 Subject: [PATCH 11/11] fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! src: make BuiltinLoader threadsafe and non-global --- src/env.cc | 3 ++- src/node_builtins.cc | 4 ---- src/node_builtins.h | 1 - 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/env.cc b/src/env.cc index 62d834a9659972..88d5b24ffc1764 100644 --- a/src/env.cc +++ b/src/env.cc @@ -676,7 +676,8 @@ Environment::Environment(IsolateData* isolate_data, ? AllocateEnvironmentThreadId().id : thread_id.id) { #ifdef NODE_V8_SHARED_RO_HEAP - if (isolate_data->worker_context() != nullptr) { + 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()); diff --git a/src/node_builtins.cc b/src/node_builtins.cc index 2b9899c3c2251b..cfb4b5ab031dba 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -669,10 +669,6 @@ void BuiltinLoader::HasCachedBuiltins(const FunctionCallbackInfo& args) { args.GetIsolate(), instance->code_cache_->has_code_cache)); } -std::unique_ptr BuiltinLoader::Create() { - return std::unique_ptr{new BuiltinLoader()}; -} - void BuiltinLoader::CopySourceAndCodeCacheReferenceFrom( const BuiltinLoader* other) { code_cache_ = other->code_cache_; diff --git a/src/node_builtins.h b/src/node_builtins.h index 53c323455fca72..81d6d1cca279c2 100644 --- a/src/node_builtins.h +++ b/src/node_builtins.h @@ -78,7 +78,6 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { void RefreshCodeCache(const std::vector& in); void CopyCodeCache(std::vector* out) const; - static std::unique_ptr Create(); void CopySourceAndCodeCacheReferenceFrom(const BuiltinLoader* other); private: