From 6ff99f105107b527a0711e36011e713998ead1b5 Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Wed, 15 Mar 2023 05:09:04 +0000 Subject: [PATCH] address code review --- node.gyp | 1 - src/node_builtins.cc | 103 ++++++++++++++------------------ src/node_builtins.h | 17 +++--- src/node_eternal_bytes.h | 97 ------------------------------ src/node_union_bytes.h | 83 +++++++++++++++++-------- src/util.cc | 62 ++----------------- test/cctest/test_per_process.cc | 18 +++--- tools/js2c.py | 22 +++---- 8 files changed, 139 insertions(+), 264 deletions(-) delete mode 100644 src/node_eternal_bytes.h diff --git a/node.gyp b/node.gyp index fa22edd0b8fe1b..72d90ec5f4ad14 100644 --- a/node.gyp +++ b/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', diff --git a/src/node_builtins.cc b/src/node_builtins.cc index 00cd94f032fad7..95d63116d0ec29 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -34,7 +34,8 @@ using v8::Undefined; using v8::Value; BuiltinLoader::BuiltinLoader() - : code_cache_(std::make_shared()) { + : config_(GetConfig()), code_cache_(std::make_shared()) { + 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 BuiltinLoader::GetSourceObject(Local context) { - auto internalized_builtins = GetInternalizedBuiltinSourceMap(); Isolate* isolate = context->GetIsolate(); Local out = Object::New(isolate); - for (auto const& x : *internalized_builtins) { - Local 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 key = OneByteString(isolate, x.first.c_str(), x.first.size()); @@ -90,17 +76,13 @@ Local BuiltinLoader::GetSourceObject(Local context) { } Local BuiltinLoader::GetConfigString(Isolate* isolate) { - return GetConfig()->ToStringChecked(isolate); + return config_.ToStringChecked(isolate); } std::vector BuiltinLoader::GetBuiltinIds() const { std::vector 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 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 BuiltinLoader::LoadBuiltinSource(Isolate* isolate, namespace { static Mutex externalized_builtins_mutex; -std::unordered_map externalized_builtin_sources; +std::unordered_map> + 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>(expected_u16_length); + size_t u16_length = simdutf::convert_utf8_to_utf16( + source.data(), + source.length(), + reinterpret_cast(out->data())); + out->resize(u16_length); + + auto resource_ptr = std::make_unique( + 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>(expected_u16_length); - size_t u16_length = - simdutf::convert_utf8_to_utf16(utf8source.data(), - utf8source.length(), - reinterpret_cast(out->data())); - out->resize(u16_length); - return Add(id, UnionBytes(out)); + Add(id, UnionBytes(resource)); } // Returns Local 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()); + } } } diff --git a/src/node_builtins.h b/src/node_builtins.h index 25096e2521f54a..1fc7c8ee44d266 100644 --- a/src/node_builtins.h +++ b/src/node_builtins.h @@ -10,7 +10,6 @@ #include #include #include -#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; -using NonInternalizedBuiltinSourceMap = std::map; +using BuiltinSourceMap = std::map; using BuiltinCodeCacheMap = std::unordered_map>; -const InternalizedBuiltinSourceMap* GetInternalizedBuiltinSourceMap(); -const EternalBytes* GetConfig(); +// Generated by tools/js2c.py as node_javascript.cc +const ThreadsafeCopyOnWrite* 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 GetBuiltinIds() const; struct BuiltinCategories { @@ -134,10 +136,9 @@ class NODE_EXTERN_PRIVATE BuiltinLoader { void AddExternalizedBuiltin(const char* id, const char* filename); - ThreadsafeCopyOnWrite source_; + ThreadsafeCopyOnWrite source_; - static void RegisterSourcesAsExternalReferences( - ExternalReferenceRegistry* registry); + const UnionBytes config_; struct BuiltinCodeCache { RwLock mutex; diff --git a/src/node_eternal_bytes.h b/src/node_eternal_bytes.h deleted file mode 100644 index 89fb176d3a18ca..00000000000000 --- a/src/node_eternal_bytes.h +++ /dev/null @@ -1,97 +0,0 @@ -#ifndef SRC_NODE_ETERNAL_BYTES_H_ -#define SRC_NODE_ETERNAL_BYTES_H_ - -#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS - -// A pointer to const uint8_t* or const uint16_t* data that can be turned into -// external v8::String when given an isolate. Unlike UnionBytes, this assumes -// that the underlying buffer is eternal (will not be garbage collected) and -// reuses the same v8::String::ExternalStringResourceBase for all materialized -// strings. This allows the resource to be registered as an external reference -// for snapshotting. - -#include -#include "v8.h" - -namespace node { - -class EternalBytes; - -template -class EternalExternalByteResource : public Base { - static_assert(sizeof(IChar) == sizeof(Char), - "incompatible interface and internal pointers"); - - public: - explicit EternalExternalByteResource(const Char* data, size_t length) - : data_(data), length_(length) {} - - const IChar* data() const override { - return reinterpret_cast(data_); - } - size_t length() const override { return length_; } - - void Dispose() override { - // Do nothing. This class is owned by the EternalBytes instance and so - // should not be destroyed. It may also be in use by other external strings - // besides the one which was collected. - } - - EternalExternalByteResource(const EternalExternalByteResource&) = delete; - EternalExternalByteResource& operator=(const EternalExternalByteResource&) = - delete; - - private: - const Char* data_; - const size_t length_; -}; - -using EternalExternalOneByteResource = - EternalExternalByteResource; -using EternalExternalTwoByteResource = - EternalExternalByteResource; - -class EternalBytes { - public: - explicit EternalBytes(EternalExternalOneByteResource* one_byte_resource) - : one_byte_resource_(one_byte_resource), two_byte_resource_(nullptr) {} - explicit EternalBytes(EternalExternalTwoByteResource* two_byte_resource) - : one_byte_resource_(nullptr), two_byte_resource_(two_byte_resource) {} - - EternalBytes(const EternalBytes&) = default; - EternalBytes& operator=(const EternalBytes&) = default; - EternalBytes(EternalBytes&&) = default; - EternalBytes& operator=(EternalBytes&&) = default; - - bool IsOneByte() const { return one_byte_resource_ != nullptr; } - - const v8::String::ExternalStringResourceBase* AsResource() const { - return IsOneByte() - ? static_cast( - one_byte_resource_) - : static_cast( - two_byte_resource_); - } - - v8::Local ToStringChecked(v8::Isolate* isolate) const { - return IsOneByte() - ? v8::String::NewExternalOneByte(isolate, one_byte_resource_) - .ToLocalChecked() - : v8::String::NewExternalTwoByte(isolate, two_byte_resource_) - .ToLocalChecked(); - } - - private: - EternalExternalOneByteResource* one_byte_resource_; - EternalExternalTwoByteResource* two_byte_resource_; -}; - -} // namespace node - -#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS - -#endif // SRC_NODE_ETERNAL_BYTES_H_ diff --git a/src/node_union_bytes.h b/src/node_union_bytes.h index 6e1a3afb614323..d23b73013a86e7 100644 --- a/src/node_union_bytes.h +++ b/src/node_union_bytes.h @@ -4,50 +4,85 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -// A union of const uint8_t* or const uint16_t* data that can be -// turned into external v8::String when given an isolate. - #include "v8.h" namespace node { +// An external resource intended to be used with static lifetime. +template +class StaticExternalByteResource : public Base { + static_assert(sizeof(IChar) == sizeof(Char), + "incompatible interface and internal pointers"); + + public: + explicit StaticExternalByteResource(const Char* data, + size_t length, + std::shared_ptr owning_ptr) + : data_(data), length_(length), owning_ptr_(owning_ptr) {} + + const IChar* data() const override { + return reinterpret_cast(data_); + } + size_t length() const override { return length_; } + + bool IsDataStatic() const { return owning_ptr_ == nullptr; } + + void Dispose() override { + // We ignore Dispose calls from V8, even if we "own" a resource via + // owning_ptr_. All instantiations of this class are static or owned by a + // static map, and will be destructed when static variables are destructed. + } + + StaticExternalByteResource(const StaticExternalByteResource&) = delete; + StaticExternalByteResource& operator=(const StaticExternalByteResource&) = + delete; + + private: + const Char* data_; + const size_t length_; + std::shared_ptr owning_ptr_; +}; + +using StaticExternalOneByteResource = + StaticExternalByteResource; +using StaticExternalTwoByteResource = + StaticExternalByteResource; + // Similar to a v8::String, but it's independent from Isolates // and can be materialized in Isolates as external Strings // via ToStringChecked. class UnionBytes { public: - UnionBytes(const uint16_t* data, size_t length) - : one_bytes_(nullptr), two_bytes_(data), length_(length) {} - UnionBytes(const uint8_t* data, size_t length) - : one_bytes_(data), two_bytes_(nullptr), length_(length) {} - template // T = uint8_t or uint16_t - explicit UnionBytes(std::shared_ptr> data) - : UnionBytes(data->data(), data->size()) { - owning_ptr_ = data; - } + UnionBytes(StaticExternalOneByteResource* one_byte_resource) + : one_byte_resource_(one_byte_resource), two_byte_resource_(nullptr) {} + UnionBytes(StaticExternalTwoByteResource* two_byte_resource) + : one_byte_resource_(nullptr), two_byte_resource_(two_byte_resource) {} UnionBytes(const UnionBytes&) = default; UnionBytes& operator=(const UnionBytes&) = default; UnionBytes(UnionBytes&&) = default; UnionBytes& operator=(UnionBytes&&) = default; - bool is_one_byte() const { return one_bytes_ != nullptr; } - const uint16_t* two_bytes_data() const { - CHECK_NOT_NULL(two_bytes_); - return two_bytes_; + bool IsOneByte() const { return one_byte_resource_ != nullptr; } + bool IsDataStatic() const { + return IsOneByte() ? one_byte_resource_->IsDataStatic() + : two_byte_resource_->IsDataStatic(); } - const uint8_t* one_bytes_data() const { - CHECK_NOT_NULL(one_bytes_); - return one_bytes_; + v8::String::ExternalStringResourceBase* AsResource() const { + return IsOneByte() ? static_cast( + one_byte_resource_) + : static_cast( + two_byte_resource_); } v8::Local ToStringChecked(v8::Isolate* isolate) const; - size_t length() const { return length_; } private: - const uint8_t* one_bytes_; - const uint16_t* two_bytes_; - size_t length_; - std::shared_ptr owning_ptr_; + StaticExternalOneByteResource* one_byte_resource_; + StaticExternalTwoByteResource* two_byte_resource_; }; } // namespace node diff --git a/src/util.cc b/src/util.cc index a50f780b73664f..46bf6f51353f6c 100644 --- a/src/util.cc +++ b/src/util.cc @@ -551,65 +551,13 @@ void SetConstructorFunction(Isolate* isolate, that->Set(name, tmpl); } -namespace { - -class NonOwningExternalOneByteResource - : public v8::String::ExternalOneByteStringResource { - public: - explicit NonOwningExternalOneByteResource(const UnionBytes& source) - : source_(source) {} - ~NonOwningExternalOneByteResource() override = default; - - const char* data() const override { - return reinterpret_cast(source_.one_bytes_data()); - } - size_t length() const override { return source_.length(); } - - NonOwningExternalOneByteResource(const NonOwningExternalOneByteResource&) = - delete; - NonOwningExternalOneByteResource& operator=( - const NonOwningExternalOneByteResource&) = delete; - - private: - const UnionBytes source_; -}; - -class NonOwningExternalTwoByteResource - : public v8::String::ExternalStringResource { - public: - explicit NonOwningExternalTwoByteResource(const UnionBytes& source) - : source_(source) {} - ~NonOwningExternalTwoByteResource() override = default; - - const uint16_t* data() const override { return source_.two_bytes_data(); } - size_t length() const override { return source_.length(); } - - NonOwningExternalTwoByteResource(const NonOwningExternalTwoByteResource&) = - delete; - NonOwningExternalTwoByteResource& operator=( - const NonOwningExternalTwoByteResource&) = delete; - - private: - const UnionBytes source_; -}; - -} // anonymous namespace - Local UnionBytes::ToStringChecked(Isolate* isolate) const { - if (UNLIKELY(length() == 0)) { - // V8 requires non-null data pointers for empty external strings, - // but we don't guarantee that. Solve this by not creating an - // external string at all in that case. - return String::Empty(isolate); - } - if (is_one_byte()) { - NonOwningExternalOneByteResource* source = - new NonOwningExternalOneByteResource(*this); - return String::NewExternalOneByte(isolate, source).ToLocalChecked(); + if (IsOneByte()) { + return String::NewExternalOneByte(isolate, one_byte_resource_) + .ToLocalChecked(); } else { - NonOwningExternalTwoByteResource* source = - new NonOwningExternalTwoByteResource(*this); - return String::NewExternalTwoByte(isolate, source).ToLocalChecked(); + return String::NewExternalTwoByte(isolate, two_byte_resource_) + .ToLocalChecked(); } } diff --git a/test/cctest/test_per_process.cc b/test/cctest/test_per_process.cc index 822bfb3a6cf2f5..8894411fcfa20f 100644 --- a/test/cctest/test_per_process.cc +++ b/test/cctest/test_per_process.cc @@ -7,28 +7,26 @@ #include using node::builtins::BuiltinLoader; -using node::builtins::InternalizedBuiltinSourceMap; +using node::builtins::BuiltinSourceMap; class PerProcessTest : public ::testing::Test { protected: - static const InternalizedBuiltinSourceMap* - get_internalized_builtin_source_map_for_test() { - return node::builtins::GetInternalizedBuiltinSourceMap(); + static const BuiltinSourceMap get_sources_for_test() { + return *BuiltinLoader().source_.read(); } }; namespace { TEST_F(PerProcessTest, EmbeddedSources) { - const auto& sources = - PerProcessTest::get_internalized_builtin_source_map_for_test(); - ASSERT_TRUE(std::any_of(sources->cbegin(), sources->cend(), [](auto p) { + const auto& sources = PerProcessTest::get_sources_for_test(); + ASSERT_TRUE(std::any_of(sources.cbegin(), sources.cend(), [](auto p) { return p.second.IsOneByte(); - })) << "internalized_builtin_source_map should have some 8bit items"; + })) << "BuiltinLoader::source_ should have some 8bit items"; - ASSERT_TRUE(std::any_of(sources->cbegin(), sources->cend(), [](auto p) { + ASSERT_TRUE(std::any_of(sources.cbegin(), sources.cend(), [](auto p) { return !p.second.IsOneByte(); - })) << "internalized_builtin_source_map should have some 16bit items"; + })) << "BuiltinLoader::source_ should have some 16bit items"; } } // end namespace diff --git a/tools/js2c.py b/tools/js2c.py index 93a1837a6974df..e8bf4c52e49ed1 100755 --- a/tools/js2c.py +++ b/tools/js2c.py @@ -58,19 +58,21 @@ def ReadFile(filename): {0} namespace {{ -const InternalizedBuiltinSourceMap internalized_builtin_source_map {{ - InternalizedBuiltinSourceMap{{ {1} }} +const ThreadsafeCopyOnWrite global_source_map {{ + BuiltinSourceMap{{ {1} }} }}; +}} -const EternalBytes config(&{2}); +void BuiltinLoader::LoadJavaScriptSource() {{ + source_ = global_source_map; }} -const InternalizedBuiltinSourceMap* GetInternalizedBuiltinSourceMap() {{ - return &internalized_builtin_source_map; +const ThreadsafeCopyOnWrite *GetGlobalSourceMap() {{ + return &global_source_map; }} -const EternalBytes* GetConfig() {{ - return &config; +UnionBytes BuiltinLoader::GetConfig() {{ + return UnionBytes(&{2}); }} }} // namespace builtins @@ -83,7 +85,7 @@ def ReadFile(filename): {1} }}; -static EternalExternalOneByteResource {2}({0}, {3}); +static StaticExternalOneByteResource {2}({0}, {3}, nullptr); """ TWO_BYTE_STRING = """ @@ -91,10 +93,10 @@ def ReadFile(filename): {1} }}; -static EternalExternalTwoByteResource {2}({0}, {3}); +static StaticExternalTwoByteResource {2}({0}, {3}, nullptr); """ -INITIALIZER = '{{"{0}", EternalBytes(&{1}) }},' +INITIALIZER = '{{"{0}", UnionBytes(&{1}) }},' CONFIG_GYPI_ID = 'config_raw'