Skip to content

Commit

Permalink
src: use an array for faster binding data lookup
Browse files Browse the repository at this point in the history
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: nodejs/node#46620
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
  • Loading branch information
sercher committed Apr 25, 2024
1 parent dbb6d98 commit c334df1
Show file tree
Hide file tree
Showing 17 changed files with 49 additions and 62 deletions.
1 change: 1 addition & 0 deletions graal-nodejs/node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
30 changes: 19 additions & 11 deletions graal-nodejs/src/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<Object> obj) : BaseObject(realm, obj) {}

static constexpr FastStringKey type_name { "http_parser" };
SET_BINDING_ID(http_parser_binding_data)

std::vector<char> parser_buffer;
bool parser_buffer_in_use = false;
Expand Down Expand Up @@ -569,12 +583,6 @@ void InitializeHttpParser(Local<Object> 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.
<a id="exception-handling"></a>
### Exception handling
Expand Down
1 change: 1 addition & 0 deletions graal-nodejs/src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include <type_traits> // std::remove_reference
#include "base_object_types.h"
#include "memory_tracker.h"
#include "v8.h"

Expand Down
2 changes: 0 additions & 2 deletions graal-nodejs/src/base_object_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
4 changes: 1 addition & 3 deletions graal-nodejs/src/node_blob.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 1 addition & 3 deletions graal-nodejs/src/node_file.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion graal-nodejs/src/node_http2_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion graal-nodejs/src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class BindingData : public BaseObject {
public:
BindingData(Realm* realm, Local<Object> obj) : BaseObject(realm, obj) {}

static constexpr FastStringKey type_name { "http_parser" };
SET_BINDING_ID(http_parser_binding_data)

std::vector<char> parser_buffer;
bool parser_buffer_in_use = false;
Expand Down
4 changes: 1 addition & 3 deletions graal-nodejs/src/node_process.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Object> object);

Expand Down
14 changes: 9 additions & 5 deletions graal-nodejs/src/node_realm-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,11 @@ inline T* Realm::GetBindingData(v8::Local<v8::Context> context) {
static_cast<BindingDataStore*>(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<T*>(it->second.get());
constexpr size_t binding_index = static_cast<size_t>(T::binding_type_int);
static_assert(binding_index < std::tuple_size_v<BindingDataStore>);
auto ptr = (*map)[binding_index];
if (UNLIKELY(!ptr)) return nullptr;
T* result = static_cast<T*>(ptr.get());
DCHECK_NOT_NULL(result);
DCHECK_EQ(result->realm(), GetCurrent(context));
return result;
Expand All @@ -80,8 +82,10 @@ inline T* Realm::AddBindingData(v8::Local<v8::Context> context,
static_cast<BindingDataStore*>(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<size_t>(T::binding_type_int);
static_assert(binding_index < std::tuple_size_v<BindingDataStore>);
CHECK(!(*map)[binding_index]); // Should not insert the binding twice.
(*map)[binding_index] = item;
DCHECK_EQ(GetBindingData<T>(context), item.get());
return item.get();
}
Expand Down
5 changes: 3 additions & 2 deletions graal-nodejs/src/node_realm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
6 changes: 3 additions & 3 deletions graal-nodejs/src/node_realm.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ struct RealmSerializeInfo {
friend std::ostream& operator<<(std::ostream& o, const RealmSerializeInfo& i);
};

using BindingDataStore = std::unordered_map<FastStringKey,
BaseObjectPtr<BaseObject>,
FastStringKey::Hash>;
using BindingDataStore = std::array<BaseObjectPtr<BaseObject>,
static_cast<size_t>(
BindingDataType::kBindingDataTypeCount)>;

/**
* node::Realm is a container for a set of JavaScript objects and functions
Expand Down
8 changes: 4 additions & 4 deletions graal-nodejs/src/node_snapshotable.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1353,7 +1353,7 @@ void DeserializeNodeInternalFields(Local<Object> 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, \
Expand Down Expand Up @@ -1440,7 +1440,7 @@ void SerializeSnapshotableObjects(Realm* realm,
}
SnapshotableObject* ptr = static_cast<SnapshotableObject*>(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",
Expand Down
16 changes: 1 addition & 15 deletions graal-nodejs/src/node_snapshotable.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -102,7 +88,7 @@ class SnapshotableObject : public BaseObject {
SnapshotableObject(Realm* realm,
v8::Local<v8::Object> 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<v8::Context> context,
Expand Down
4 changes: 1 addition & 3 deletions graal-nodejs/src/node_url.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 1 addition & 3 deletions graal-nodejs/src/node_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Object> object,
Expand Down
4 changes: 1 addition & 3 deletions graal-nodejs/src/node_v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit c334df1

Please sign in to comment.