fixup! src: make BuiltinLoader threadsafe and non-global

nodejs · nodejs-github-bot · Jan 18, 2023 · Dec 22, 2022 · Jan 6, 2023 · Jan 6, 2023
commit 2d3190ce31106d7e957f89b89c2f320dba2d49cd
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -436,14 +436,9 @@ inline std::vector<double>* Environment::destroy_async_id_list() {
   return &destroy_async_id_list_;
 }
 
-inline std::shared_ptr<builtins::BuiltinLoader> Environment::builtin_loader() {
+inline builtins::BuiltinLoader* Environment::builtin_loader() {
   DCHECK(builtin_loader_);
-  return builtin_loader_;
-}
-
-inline void Environment::set_builtin_loader(
-    std::shared_ptr<builtins::BuiltinLoader> loader) {
-  builtin_loader_ = loader;
+  return builtin_loader_.get();
 }
 
 inline double Environment::new_async_id() {

diff --git 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<uint64_t>(-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
@@ -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<double>* destroy_async_id_list();
 
-  std::shared_ptr<builtins::BuiltinLoader> builtin_loader();
-  void set_builtin_loader(std::shared_ptr<builtins::BuiltinLoader> loader);
+  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;
@@ -1147,7 +1146,7 @@ class Environment : public MemoryRetainer {
 
   std::unique_ptr<Realm> principal_realm_ = nullptr;
 
-  std::shared_ptr<builtins::BuiltinLoader> builtin_loader_;
+  std::unique_ptr<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.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<Object> BuiltinLoader::GetSourceObject(Local<Context> context) {
-  RwLock::ScopedReadLock lock(source_mutex_);
   Isolate* isolate = context->GetIsolate();
   Local<Object> out = Object::New(isolate);
-  for (auto const& x : source_) {
+  auto source = source_.read();
+  for (auto const& x : *source) {
     Local<String> key = OneByteString(isolate, x.first.c_str(), x.first.size());
     out->Set(context, key, x.second.ToStringChecked(isolate)).FromJust();
   }
@@ -82,18 +81,17 @@ Local<String> BuiltinLoader::GetConfigString(Isolate* isolate) {
 }
 
 std::vector<std::string> BuiltinLoader::GetBuiltinIds() const {
-  RwLock::ScopedReadLock lock(source_mutex_);
   std::vector<std::string> 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;
 }
 
 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<String> 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<Function> BuiltinLoader::LookupAndCompile(Local<Context> context,
   MaybeLocal<Function> maybe =
       LookupAndCompileInternal(context, id, &parameters, &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> context) {
   return all_succeeded;
 }
 
-void BuiltinLoader::CopyCodeCache(std::vector<CodeCacheInfo>* out) {
+void BuiltinLoader::CopyCodeCache(std::vector<CodeCacheInfo>* 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<Value>& args) {
       v8::Boolean::New(args.GetIsolate(), instance->has_code_cache_));
 }
 
-std::shared_ptr<BuiltinLoader> BuiltinLoader::Create() {
-  return std::shared_ptr<BuiltinLoader>{new BuiltinLoader()};
+std::unique_ptr<BuiltinLoader> BuiltinLoader::Create() {
+  return std::unique_ptr<BuiltinLoader>{new BuiltinLoader()};
+}
+
+void BuiltinLoader::CopySourceAndCodeCacheFrom(const BuiltinLoader* other) {
+  {
+    std::vector<CodeCacheInfo> 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
@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 #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<v8::Context> context);
   void RefreshCodeCache(const std::vector<CodeCacheInfo>& in);
-  void CopyCodeCache(std::vector<CodeCacheInfo>* out);
+  void CopyCodeCache(std::vector<CodeCacheInfo>* out) const;
 
-  static std::shared_ptr<BuiltinLoader> Create();
+  static std::unique_ptr<BuiltinLoader> 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<v8::String> 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<BuiltinCategories> builtin_categories_;
 
-  // Used to synchronize access to the code cache map
-  RwLock source_mutex_;
-  BuiltinSourceMap source_;
+  ThreadsafeCopyOnWrite<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_;

diff --git 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
@@ -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 <typename T>
+T* CopyOnWrite<T>::write() {
+  if (!data_.unique()) {
+    data_ = std::make_shared<T>(*data_);
+  }
+  return data_.get();
+}
+
+template <typename T>
+ThreadsafeCopyOnWrite<T>::Read::Read(const ThreadsafeCopyOnWrite<T>* cow)
+    : cow_(cow), lock_(cow->impl_->mutex) {}
+
+template <typename T>
+const T& ThreadsafeCopyOnWrite<T>::Read::operator*() const {
+  return cow_->impl_->data;
+}
+
+template <typename T>
+const T* ThreadsafeCopyOnWrite<T>::Read::operator->() const {
+  return &cow_->impl_->data;
+}
+
+template <typename T>
+ThreadsafeCopyOnWrite<T>::Write::Write(ThreadsafeCopyOnWrite<T>* cow)
+    : cow_(cow), impl_(cow->impl_.write()), lock_(impl_->mutex) {}
+
+template <typename T>
+T& ThreadsafeCopyOnWrite<T>::Write::operator*() {
+  return impl_->data;
+}
+
+template <typename T>
+T* ThreadsafeCopyOnWrite<T>::Write::operator->() {
+  return &impl_->data;
+}
+
+template <typename T>
+ThreadsafeCopyOnWrite<T>::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_