address code review

node.gyp

@@ -617,7 +617,6 @@
         'src/node_contextify.h',
         'src/node_dir.h',
         'src/node_errors.h',
-        'src/node_eternal_bytes.h',
         'src/node_exit_code.h',
         'src/node_external_reference.h',
         'src/node_file.h',

src/node_builtins.cc

@@ -34,7 +34,8 @@ using v8::Undefined;
 using v8::Value;
 
 BuiltinLoader::BuiltinLoader()
-    : code_cache_(std::make_shared<BuiltinCodeCache>()) {
+    : config_(GetConfig()), code_cache_(std::make_shared<BuiltinCodeCache>()) {
+  LoadJavaScriptSource();
 #ifdef NODE_SHARED_BUILTIN_CJS_MODULE_LEXER_LEXER_PATH
   AddExternalizedBuiltin(
       "internal/deps/cjs-module-lexer/lexer",
@@ -54,33 +55,18 @@ BuiltinLoader::BuiltinLoader()
 }
 
 bool BuiltinLoader::Exists(const char* id) {
-  auto internalized_builtins = GetInternalizedBuiltinSourceMap();
-  if (internalized_builtins->find(id) != internalized_builtins->end()) {
-    return true;
-  }
   auto source = source_.read();
   return source->find(id) != source->end();
 }
 
 bool BuiltinLoader::Add(const char* id, const UnionBytes& source) {
-  auto internalized_builtins = GetInternalizedBuiltinSourceMap();
-  if (internalized_builtins->find(id) != internalized_builtins->end()) {
-    // Cannot add this builtin as it would conflict with an
-    // existing internalized builtin.
-    return false;
-  }
   auto result = source_.write()->emplace(id, source);
   return result.second;
 }
 
 Local<Object> BuiltinLoader::GetSourceObject(Local<Context> context) {
-  auto internalized_builtins = GetInternalizedBuiltinSourceMap();
   Isolate* isolate = context->GetIsolate();
   Local<Object> out = Object::New(isolate);
-  for (auto const& x : *internalized_builtins) {
-    Local<String> key = OneByteString(isolate, x.first.c_str(), x.first.size());
-    out->Set(context, key, x.second.ToStringChecked(isolate)).FromJust();
-  }
   auto source = source_.read();
   for (auto const& x : *source) {
     Local<String> key = OneByteString(isolate, x.first.c_str(), x.first.size());
@@ -90,17 +76,13 @@ Local<Object> BuiltinLoader::GetSourceObject(Local<Context> context) {
 }
 
 Local<String> BuiltinLoader::GetConfigString(Isolate* isolate) {
-  return GetConfig()->ToStringChecked(isolate);
+  return config_.ToStringChecked(isolate);
 }
 
 std::vector<std::string> BuiltinLoader::GetBuiltinIds() const {
   std::vector<std::string> ids;
-  auto internalized_builtins = GetInternalizedBuiltinSourceMap();
   auto source = source_.read();
-  ids.reserve(internalized_builtins->size() + source->size());
-  for (auto const& x : *internalized_builtins) {
-    ids.emplace_back(x.first);
-  }
+  ids.reserve(source->size());
   for (auto const& x : *source) {
     ids.emplace_back(x.first);
   }
@@ -202,20 +184,14 @@ static std::string OnDiskFileName(const char* id) {
 
 MaybeLocal<String> BuiltinLoader::LoadBuiltinSource(Isolate* isolate,
                                                     const char* id) const {
-#ifndef NODE_BUILTIN_MODULES_PATH
-  auto internalized_builtins = GetInternalizedBuiltinSourceMap();
-  const auto internalized_builtins_it = internalized_builtins->find(id);
-  if (LIKELY(internalized_builtins_it != internalized_builtins->end())) {
-    return internalized_builtins_it->second.ToStringChecked(isolate);
-  }
-
   auto source = source_.read();
-  auto source_it = source->find(id);
-  if (LIKELY(source_it != source->end())) {
-    return source_it->second.ToStringChecked(isolate);
+#ifndef NODE_BUILTIN_MODULES_PATH
+  const auto source_it = source->find(id);
+  if (UNLIKELY(source_it == source->end())) {
+    fprintf(stderr, "Cannot find native builtin: \"%s\".\n", id);
+    ABORT();
   }
-  fprintf(stderr, "Cannot find native builtin: \"%s\".\n", id);
-  ABORT();
+  return source_it->second.ToStringChecked(isolate);
 #else   // !NODE_BUILTIN_MODULES_PATH
   std::string filename = OnDiskFileName(id);
 
@@ -237,19 +213,23 @@ MaybeLocal<String> BuiltinLoader::LoadBuiltinSource(Isolate* isolate,
 
 namespace {
 static Mutex externalized_builtins_mutex;
-std::unordered_map<std::string, std::string> externalized_builtin_sources;
+std::unordered_map<std::string, std::unique_ptr<StaticExternalTwoByteResource>>
+    externalized_builtin_sources;
 }  // namespace
 
 void BuiltinLoader::AddExternalizedBuiltin(const char* id,
                                            const char* filename) {
-  std::string source;
+  StaticExternalTwoByteResource* resource;
   {
     Mutex::ScopedLock lock(externalized_builtins_mutex);
     auto it = externalized_builtin_sources.find(id);
     if (it != externalized_builtin_sources.end()) {
-      source = it->second;
-    }
-    {
+      // OK to get the raw pointer, since externalized_builtin_sources owns
+      // the resource, resources are never removed from the map, and
+      // externalized_builtin_sources has static lifetime.
+      resource = it->second.get();
+    } else {
+      std::string source;
       int r = ReadFileSync(&source, filename);
       if (r != 0) {
         fprintf(stderr,
@@ -258,23 +238,26 @@ void BuiltinLoader::AddExternalizedBuiltin(const char* id,
                 filename);
         ABORT();
       }
-      externalized_builtin_sources[id] = source;
+      size_t expected_u16_length =
+          simdutf::utf16_length_from_utf8(source.data(), source.length());
+      auto out = std::make_shared<std::vector<uint16_t>>(expected_u16_length);
+      size_t u16_length = simdutf::convert_utf8_to_utf16(
+          source.data(),
+          source.length(),
+          reinterpret_cast<char16_t*>(out->data()));
+      out->resize(u16_length);
+
+      auto resource_ptr = std::make_unique<StaticExternalTwoByteResource>(
+          out->data(), out->size(), out);
+      // OK to get the raw pointer, since externalized_builtin_sources owns
+      // the resource, resources are never removed from the map, and
+      // externalized_builtin_sources has static lifetime.
+      resource = resource_ptr.get();
+      externalized_builtin_sources[id] = std::move(resource_ptr);
     }
   }
 
-  Add(id, source);
-}
-
-bool BuiltinLoader::Add(const char* id, std::string_view utf8source) {
-  size_t expected_u16_length =
-      simdutf::utf16_length_from_utf8(utf8source.data(), utf8source.length());
-  auto out = std::make_shared<std::vector<uint16_t>>(expected_u16_length);
-  size_t u16_length =
-      simdutf::convert_utf8_to_utf16(utf8source.data(),
-                                     utf8source.length(),
-                                     reinterpret_cast<char16_t*>(out->data()));
-  out->resize(u16_length);
-  return Add(id, UnionBytes(out));
+  Add(id, UnionBytes(resource));
 }
 
 // Returns Local<Function> of the compiled module if return_code_cache
@@ -734,10 +717,16 @@ void BuiltinLoader::RegisterExternalReferences(
   registry->Register(GetCacheUsage);
   registry->Register(CompileFunction);
   registry->Register(HasCachedBuiltins);
-  auto internalized_builtins = GetInternalizedBuiltinSourceMap();
-  for (auto const& x : *internalized_builtins) {
-    auto resource = x.second.AsResource();
-    registry->Register(resource);
+
+  auto global_sources = GetGlobalSourceMap()->read();
+  for (auto const& x : *global_sources) {
+    // We can register any static data as an external reference, since (1) we
+    // know it'll survive until snapshot serialization or deserialization and
+    // (2) the list of static data is guaranteed to be in a fixed order (since
+    // resources with static data are internalized builtins created by js2c).
+    if (x.second.IsDataStatic()) {
+      registry->Register(x.second.AsResource());
+    }
   }
 }
 

src/node_builtins.h

@@ -10,7 +10,6 @@
 #include <set>
 #include <string>
 #include <vector>
-#include "node_eternal_bytes.h"
 #include "node_mutex.h"
 #include "node_threadsafe_cow.h"
 #include "node_union_bytes.h"
@@ -26,14 +25,13 @@ class Realm;
 
 namespace builtins {
 
-using InternalizedBuiltinSourceMap = std::map<std::string, EternalBytes>;
-using NonInternalizedBuiltinSourceMap = std::map<std::string, UnionBytes>;
+using BuiltinSourceMap = std::map<std::string, UnionBytes>;
 using BuiltinCodeCacheMap =
     std::unordered_map<std::string,
                        std::unique_ptr<v8::ScriptCompiler::CachedData>>;
 
-const InternalizedBuiltinSourceMap* GetInternalizedBuiltinSourceMap();
-const EternalBytes* GetConfig();
+// Generated by tools/js2c.py as node_javascript.cc
+const ThreadsafeCopyOnWrite<BuiltinSourceMap>* GetGlobalSourceMap();
 
 struct CodeCacheInfo {
   std::string id;
@@ -89,6 +87,10 @@ class NODE_EXTERN_PRIVATE BuiltinLoader {
   // Only allow access from friends.
   friend class CodeCacheBuilder;
 
+  // Generated by tools/js2c.py as node_javascript.cc
+  void LoadJavaScriptSource();  // Loads data into source_
+  UnionBytes GetConfig();       // Return data for config.gypi
+
   std::vector<std::string> GetBuiltinIds() const;
 
   struct BuiltinCategories {
@@ -134,10 +136,9 @@ class NODE_EXTERN_PRIVATE BuiltinLoader {
 
   void AddExternalizedBuiltin(const char* id, const char* filename);
 
-  ThreadsafeCopyOnWrite<NonInternalizedBuiltinSourceMap> source_;
+  ThreadsafeCopyOnWrite<BuiltinSourceMap> source_;
 
-  static void RegisterSourcesAsExternalReferences(
-      ExternalReferenceRegistry* registry);
+  const UnionBytes config_;
 
   struct BuiltinCodeCache {
     RwLock mutex;