From c334df19e8010fd822cc32d14b7dd8e86e818087 Mon Sep 17 00:00:00 2001 From: Sergey Chernyshev Date: Thu, 25 Apr 2024 01:48:40 +0200 Subject: [PATCH] src: use an array for faster binding data lookup Locally the hashing of the binding names sometimes has significant presence in the profile of bindings, because there can be collisions, which makes the cost of adding a new binding data non-trivial, but it's wasteful to spend time on hashing them or dealing with collisions at all, since we can just use the EmbedderObjectType enum as the key, as the string names are not actually used beyond debugging purposes and can be easily matched with a macro. And since we can just use the enum as the key, we do not even need the map and can just use an array with the enum as indices for the lookup. PR-URL: https://github.com/nodejs/node/pull/46620 Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Chengzhong Wu --- graal-nodejs/node.gyp | 1 + graal-nodejs/src/README.md | 30 +++++++++++++++++---------- graal-nodejs/src/base_object.h | 1 + graal-nodejs/src/base_object_types.h | 2 -- graal-nodejs/src/node_blob.h | 4 +--- graal-nodejs/src/node_file.h | 4 +--- graal-nodejs/src/node_http2_state.h | 2 +- graal-nodejs/src/node_http_parser.cc | 2 +- graal-nodejs/src/node_process.h | 4 +--- graal-nodejs/src/node_realm-inl.h | 14 ++++++++----- graal-nodejs/src/node_realm.cc | 5 +++-- graal-nodejs/src/node_realm.h | 6 +++--- graal-nodejs/src/node_snapshotable.cc | 8 +++---- graal-nodejs/src/node_snapshotable.h | 16 +------------- graal-nodejs/src/node_url.h | 4 +--- graal-nodejs/src/node_util.h | 4 +--- graal-nodejs/src/node_v8.h | 4 +--- 17 files changed, 49 insertions(+), 62 deletions(-) diff --git a/graal-nodejs/node.gyp b/graal-nodejs/node.gyp index 9da47bf79b3..c304d8d0a3a 100644 --- a/graal-nodejs/node.gyp +++ b/graal-nodejs/node.gyp @@ -604,6 +604,7 @@ 'src/async_wrap-inl.h', 'src/base_object.h', 'src/base_object-inl.h', + 'src/base_object_types.h', 'src/base64.h', 'src/base64-inl.h', 'src/callback_queue.h', diff --git a/graal-nodejs/src/README.md b/graal-nodejs/src/README.md index e3d42921b34..15317f06ae0 100644 --- a/graal-nodejs/src/README.md +++ b/graal-nodejs/src/README.md @@ -528,18 +528,32 @@ that state is through the use of `Realm::AddBindingData`, which gives binding functions access to an object for storing such state. That object is always a [`BaseObject`][]. -Its class needs to have a static `type_name` field based on a -constant string, in order to disambiguate it from other classes of this type, -and which could e.g. match the binding's name (in the example above, that would -be `cares_wrap`). +In the binding, call `SET_BINDING_ID()` with an identifier for the binding +type. For example, for `http_parser::BindingData`, the identifier can be +`http_parser_binding_data`. + +If the binding should be supported in a snapshot, the id and the +fully-specified class name should be added to the `SERIALIZABLE_BINDING_TYPES` +list in `base_object_types.h`, and the class should implement the serialization +and deserialization methods. See the comments of `SnapshotableObject` on how to +implement them. Otherwise, add the id and the class name to the +`UNSERIALIZABLE_BINDING_TYPES` list instead. ```cpp +// In base_object_types.h, add the binding to either +// UNSERIALIZABLE_BINDING_TYPES or SERIALIZABLE_BINDING_TYPES. +// The second parameter is a descriptive name of the class, which is +// usually the fully-specified class name. + +#define UNSERIALIZABLE_BINDING_TYPES(V) \ + V(http_parser_binding_data, http_parser::BindingData) + // In the HTTP parser source code file: class BindingData : public BaseObject { public: BindingData(Realm* realm, Local obj) : BaseObject(realm, obj) {} - static constexpr FastStringKey type_name { "http_parser" }; + SET_BINDING_ID(http_parser_binding_data) std::vector parser_buffer; bool parser_buffer_in_use = false; @@ -569,12 +583,6 @@ void InitializeHttpParser(Local target, } ``` -If the binding is loaded during bootstrap, add it to the -`SERIALIZABLE_OBJECT_TYPES` list in `src/node_snapshotable.h` and -inherit from the `SnapshotableObject` class instead. See the comments -of `SnapshotableObject` on how to implement its serialization and -deserialization. - ### Exception handling diff --git a/graal-nodejs/src/base_object.h b/graal-nodejs/src/base_object.h index 7bfcb95cfca..4ac644a034f 100644 --- a/graal-nodejs/src/base_object.h +++ b/graal-nodejs/src/base_object.h @@ -25,6 +25,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #include // std::remove_reference +#include "base_object_types.h" #include "memory_tracker.h" #include "v8.h" diff --git a/graal-nodejs/src/base_object_types.h b/graal-nodejs/src/base_object_types.h index 4916a20bbc6..09f1af75546 100644 --- a/graal-nodejs/src/base_object_types.h +++ b/graal-nodejs/src/base_object_types.h @@ -10,12 +10,10 @@ namespace node { // what the class passes to SET_BINDING_ID(), the second argument should match // the C++ class name. #define SERIALIZABLE_BINDING_TYPES(V) \ - V(encoding_binding_data, encoding_binding::BindingData) \ V(fs_binding_data, fs::BindingData) \ V(v8_binding_data, v8_utils::BindingData) \ V(blob_binding_data, BlobBindingData) \ V(process_binding_data, process::BindingData) \ - V(timers_binding_data, timers::BindingData) \ V(url_binding_data, url::BindingData) #define UNSERIALIZABLE_BINDING_TYPES(V) \ diff --git a/graal-nodejs/src/node_blob.h b/graal-nodejs/src/node_blob.h index d3426d05d09..08d354e2366 100644 --- a/graal-nodejs/src/node_blob.h +++ b/graal-nodejs/src/node_blob.h @@ -147,9 +147,7 @@ class BlobBindingData : public SnapshotableObject { SERIALIZABLE_OBJECT_METHODS() - static constexpr FastStringKey type_name{"node::BlobBindingData"}; - static constexpr EmbedderObjectType type_int = - EmbedderObjectType::k_blob_binding_data; + SET_BINDING_ID(blob_binding_data) void MemoryInfo(MemoryTracker* tracker) const override; SET_SELF_SIZE(BlobBindingData) diff --git a/graal-nodejs/src/node_file.h b/graal-nodejs/src/node_file.h index a4ca120d918..472b45f4611 100644 --- a/graal-nodejs/src/node_file.h +++ b/graal-nodejs/src/node_file.h @@ -69,9 +69,7 @@ class BindingData : public SnapshotableObject { using InternalFieldInfo = InternalFieldInfoBase; SERIALIZABLE_OBJECT_METHODS() - static constexpr FastStringKey type_name{"node::fs::BindingData"}; - static constexpr EmbedderObjectType type_int = - EmbedderObjectType::k_fs_binding_data; + SET_BINDING_ID(fs_binding_data) void MemoryInfo(MemoryTracker* tracker) const override; SET_SELF_SIZE(BindingData) diff --git a/graal-nodejs/src/node_http2_state.h b/graal-nodejs/src/node_http2_state.h index 75a98bf476b..f9ac6b40c34 100644 --- a/graal-nodejs/src/node_http2_state.h +++ b/graal-nodejs/src/node_http2_state.h @@ -125,7 +125,7 @@ class Http2State : public BaseObject { SET_SELF_SIZE(Http2State) SET_MEMORY_INFO_NAME(Http2State) - static constexpr FastStringKey type_name { "http2" }; + SET_BINDING_ID(http2_binding_data) private: struct http2_state_internal { diff --git a/graal-nodejs/src/node_http_parser.cc b/graal-nodejs/src/node_http_parser.cc index 2849d3bee1a..9f166ccc245 100644 --- a/graal-nodejs/src/node_http_parser.cc +++ b/graal-nodejs/src/node_http_parser.cc @@ -95,7 +95,7 @@ class BindingData : public BaseObject { public: BindingData(Realm* realm, Local obj) : BaseObject(realm, obj) {} - static constexpr FastStringKey type_name { "http_parser" }; + SET_BINDING_ID(http_parser_binding_data) std::vector parser_buffer; bool parser_buffer_in_use = false; diff --git a/graal-nodejs/src/node_process.h b/graal-nodejs/src/node_process.h index 30f655dfc71..5af062e63ea 100644 --- a/graal-nodejs/src/node_process.h +++ b/graal-nodejs/src/node_process.h @@ -54,9 +54,7 @@ class BindingData : public SnapshotableObject { using InternalFieldInfo = InternalFieldInfoBase; SERIALIZABLE_OBJECT_METHODS() - static constexpr FastStringKey type_name{"node::process::BindingData"}; - static constexpr EmbedderObjectType type_int = - EmbedderObjectType::k_process_binding_data; + SET_BINDING_ID(process_binding_data) BindingData(Realm* realm, v8::Local object); diff --git a/graal-nodejs/src/node_realm-inl.h b/graal-nodejs/src/node_realm-inl.h index b4c5896adc0..88d0818f0ae 100644 --- a/graal-nodejs/src/node_realm-inl.h +++ b/graal-nodejs/src/node_realm-inl.h @@ -62,9 +62,11 @@ inline T* Realm::GetBindingData(v8::Local context) { static_cast(context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kBindingDataStoreIndex)); DCHECK_NOT_NULL(map); - auto it = map->find(T::type_name); - if (UNLIKELY(it == map->end())) return nullptr; - T* result = static_cast(it->second.get()); + constexpr size_t binding_index = static_cast(T::binding_type_int); + static_assert(binding_index < std::tuple_size_v); + auto ptr = (*map)[binding_index]; + if (UNLIKELY(!ptr)) return nullptr; + T* result = static_cast(ptr.get()); DCHECK_NOT_NULL(result); DCHECK_EQ(result->realm(), GetCurrent(context)); return result; @@ -80,8 +82,10 @@ inline T* Realm::AddBindingData(v8::Local context, static_cast(context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kBindingDataStoreIndex)); DCHECK_NOT_NULL(map); - auto result = map->emplace(T::type_name, item); - CHECK(result.second); + constexpr size_t binding_index = static_cast(T::binding_type_int); + static_assert(binding_index < std::tuple_size_v); + CHECK(!(*map)[binding_index]); // Should not insert the binding twice. + (*map)[binding_index] = item; DCHECK_EQ(GetBindingData(context), item.get()); return item.get(); } diff --git a/graal-nodejs/src/node_realm.cc b/graal-nodejs/src/node_realm.cc index 00e0d42c3e0..a6d55c270c9 100644 --- a/graal-nodejs/src/node_realm.cc +++ b/graal-nodejs/src/node_realm.cc @@ -287,8 +287,9 @@ void Realm::DoneBootstrapping() { void Realm::RunCleanup() { TRACE_EVENT0(TRACING_CATEGORY_NODE1(realm), "RunCleanup"); - binding_data_store_.clear(); - + for (size_t i = 0; i < binding_data_store_.size(); ++i) { + binding_data_store_[i].reset(); + } cleanup_queue_.Drain(); } diff --git a/graal-nodejs/src/node_realm.h b/graal-nodejs/src/node_realm.h index 74d8c33b392..95fa6f85c70 100644 --- a/graal-nodejs/src/node_realm.h +++ b/graal-nodejs/src/node_realm.h @@ -20,9 +20,9 @@ struct RealmSerializeInfo { friend std::ostream& operator<<(std::ostream& o, const RealmSerializeInfo& i); }; -using BindingDataStore = std::unordered_map, - FastStringKey::Hash>; +using BindingDataStore = std::array, + static_cast( + BindingDataType::kBindingDataTypeCount)>; /** * node::Realm is a container for a set of JavaScript objects and functions diff --git a/graal-nodejs/src/node_snapshotable.cc b/graal-nodejs/src/node_snapshotable.cc index f70e6ddf430..e5e7ea3daf4 100644 --- a/graal-nodejs/src/node_snapshotable.cc +++ b/graal-nodejs/src/node_snapshotable.cc @@ -1308,11 +1308,11 @@ SnapshotableObject::SnapshotableObject(Realm* realm, EmbedderObjectType type) : BaseObject(realm, wrap), type_(type) {} -std::string_view SnapshotableObject::GetTypeName() const { +std::string SnapshotableObject::GetTypeName() const { switch (type_) { #define V(PropertyName, NativeTypeName) \ case EmbedderObjectType::k_##PropertyName: { \ - return NativeTypeName::type_name.as_string_view(); \ + return #NativeTypeName; \ } SERIALIZABLE_OBJECT_TYPES(V) #undef V @@ -1353,7 +1353,7 @@ void DeserializeNodeInternalFields(Local holder, per_process::Debug(DebugCategory::MKSNAPSHOT, \ "Object %p is %s\n", \ (*holder), \ - NativeTypeName::type_name.as_string_view()); \ + #NativeTypeName); \ env_ptr->EnqueueDeserializeRequest( \ NativeTypeName::Deserialize, \ holder, \ @@ -1440,7 +1440,7 @@ void SerializeSnapshotableObjects(Realm* realm, } SnapshotableObject* ptr = static_cast(obj); - std::string type_name{ptr->GetTypeName()}; + std::string type_name = ptr->GetTypeName(); per_process::Debug(DebugCategory::MKSNAPSHOT, "Serialize snapshotable object %i (%p), " "object=%p, type=%s\n", diff --git a/graal-nodejs/src/node_snapshotable.h b/graal-nodejs/src/node_snapshotable.h index 190910ced83..28d9dd8c0ae 100644 --- a/graal-nodejs/src/node_snapshotable.h +++ b/graal-nodejs/src/node_snapshotable.h @@ -22,20 +22,6 @@ struct PropInfo { SnapshotIndex index; // In the snapshot }; -#define SERIALIZABLE_OBJECT_TYPES(V) \ - V(fs_binding_data, fs::BindingData) \ - V(v8_binding_data, v8_utils::BindingData) \ - V(blob_binding_data, BlobBindingData) \ - V(process_binding_data, process::BindingData) \ - V(url_binding_data, url::BindingData) \ - V(util_weak_reference, util::WeakReference) - -enum class EmbedderObjectType : uint8_t { -#define V(PropertyName, NativeType) k_##PropertyName, - SERIALIZABLE_OBJECT_TYPES(V) -#undef V -}; - typedef size_t SnapshotIndex; // When serializing an embedder object, we'll serialize the native states @@ -102,7 +88,7 @@ class SnapshotableObject : public BaseObject { SnapshotableObject(Realm* realm, v8::Local wrap, EmbedderObjectType type); - std::string_view GetTypeName() const; + std::string GetTypeName() const; // If returns false, the object will not be serialized. virtual bool PrepareForSerialization(v8::Local context, diff --git a/graal-nodejs/src/node_url.h b/graal-nodejs/src/node_url.h index dd543a97c2e..63b4bccbe3b 100644 --- a/graal-nodejs/src/node_url.h +++ b/graal-nodejs/src/node_url.h @@ -37,9 +37,7 @@ class BindingData : public SnapshotableObject { using InternalFieldInfo = InternalFieldInfoBase; SERIALIZABLE_OBJECT_METHODS() - static constexpr FastStringKey type_name{"node::url::BindingData"}; - static constexpr EmbedderObjectType type_int = - EmbedderObjectType::k_url_binding_data; + SET_BINDING_ID(url_binding_data) void MemoryInfo(MemoryTracker* tracker) const override; SET_SELF_SIZE(BindingData) diff --git a/graal-nodejs/src/node_util.h b/graal-nodejs/src/node_util.h index 7192e080f2b..fa0faa618a6 100644 --- a/graal-nodejs/src/node_util.h +++ b/graal-nodejs/src/node_util.h @@ -14,9 +14,7 @@ class WeakReference : public SnapshotableObject { public: SERIALIZABLE_OBJECT_METHODS() - static constexpr FastStringKey type_name{"node::util::WeakReference"}; - static constexpr EmbedderObjectType type_int = - EmbedderObjectType::k_util_weak_reference; + SET_OBJECT_ID(util_weak_reference) WeakReference(Realm* realm, v8::Local object, diff --git a/graal-nodejs/src/node_v8.h b/graal-nodejs/src/node_v8.h index 2d95002bcfc..002f506d208 100644 --- a/graal-nodejs/src/node_v8.h +++ b/graal-nodejs/src/node_v8.h @@ -23,9 +23,7 @@ class BindingData : public SnapshotableObject { using InternalFieldInfo = InternalFieldInfoBase; SERIALIZABLE_OBJECT_METHODS() - static constexpr FastStringKey type_name{"node::v8::BindingData"}; - static constexpr EmbedderObjectType type_int = - EmbedderObjectType::k_v8_binding_data; + SET_BINDING_ID(v8_binding_data) AliasedFloat64Array heap_statistics_buffer; AliasedFloat64Array heap_space_statistics_buffer;