From e35ea37e2df5857a642ea65b2f8c93b3d45c71b6 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 28 Jan 2021 23:22:18 +0800 Subject: [PATCH 1/2] src: rename binding_data_name to type_name in BindingData Previously, this was a per-class string constant for BindingData which is used as keys for identifying these objects in the binding data map. These are just type names of the BindingData. This patch renames the variable to type_name so that we can generalize this constant for other BaseObjects and use it for debugging and logging the types of other BaseObjects. --- src/README.md | 4 ++-- src/env-inl.h | 4 ++-- src/node_file.cc | 2 +- src/node_file.h | 2 +- src/node_http2.cc | 2 +- src/node_http2_state.h | 2 +- src/node_http_parser.cc | 4 ++-- src/node_v8.cc | 4 ++-- 8 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/README.md b/src/README.md index aec56e7a7ba05a..a3b54416a0d2f5 100644 --- a/src/README.md +++ b/src/README.md @@ -422,7 +422,7 @@ that state is through the use of `Environment::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 `binding_data_name` field based on a +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`). @@ -433,7 +433,7 @@ class BindingData : public BaseObject { public: BindingData(Environment* env, Local obj) : BaseObject(env, obj) {} - static constexpr FastStringKey binding_data_name { "http_parser" }; + static constexpr FastStringKey type_name { "http_parser" }; std::vector parser_buffer; bool parser_buffer_in_use = false; diff --git a/src/env-inl.h b/src/env-inl.h index 4cb68e1c5dea8f..e54b75d695f763 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -358,7 +358,7 @@ inline T* Environment::GetBindingData(v8::Local context) { context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kBindingListIndex)); DCHECK_NOT_NULL(map); - auto it = map->find(T::binding_data_name); + auto it = map->find(T::type_name); if (UNLIKELY(it == map->end())) return nullptr; T* result = static_cast(it->second.get()); DCHECK_NOT_NULL(result); @@ -377,7 +377,7 @@ inline T* Environment::AddBindingData( context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kBindingListIndex)); DCHECK_NOT_NULL(map); - auto result = map->emplace(T::binding_data_name, item); + auto result = map->emplace(T::type_name, item); CHECK(result.second); DCHECK_EQ(GetBindingData(context), item.get()); return item.get(); diff --git a/src/node_file.cc b/src/node_file.cc index edf55e8766b84b..46b328b50d0b14 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2397,7 +2397,7 @@ void BindingData::MemoryInfo(MemoryTracker* tracker) const { } // TODO(addaleax): Remove once we're on C++17. -constexpr FastStringKey BindingData::binding_data_name; +constexpr FastStringKey BindingData::type_name; void Initialize(Local target, Local unused, diff --git a/src/node_file.h b/src/node_file.h index 95a68b5d89a704..9e652dc7a4afa4 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -27,7 +27,7 @@ class BindingData : public BaseObject { std::vector> file_handle_read_wrap_freelist; - static constexpr FastStringKey binding_data_name { "fs" }; + static constexpr FastStringKey type_name { "fs" }; void MemoryInfo(MemoryTracker* tracker) const override; SET_SELF_SIZE(BindingData) diff --git a/src/node_http2.cc b/src/node_http2.cc index 930167418e18db..a4b8e4d2ac7d50 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -3001,7 +3001,7 @@ void Http2State::MemoryInfo(MemoryTracker* tracker) const { } // TODO(addaleax): Remove once we're on C++17. -constexpr FastStringKey Http2State::binding_data_name; +constexpr FastStringKey Http2State::type_name; // Set up the process.binding('http2') binding. void Initialize(Local target, diff --git a/src/node_http2_state.h b/src/node_http2_state.h index cd29b207498dc0..7cf40ff1017ca3 100644 --- a/src/node_http2_state.h +++ b/src/node_http2_state.h @@ -127,7 +127,7 @@ class Http2State : public BaseObject { SET_SELF_SIZE(Http2State) SET_MEMORY_INFO_NAME(Http2State) - static constexpr FastStringKey binding_data_name { "http2" }; + static constexpr FastStringKey type_name { "http2" }; private: struct http2_state_internal { diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index affc66585ed89a..d3184bb1a14c92 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -88,7 +88,7 @@ class BindingData : public BaseObject { BindingData(Environment* env, Local obj) : BaseObject(env, obj) {} - static constexpr FastStringKey binding_data_name { "http_parser" }; + static constexpr FastStringKey type_name { "http_parser" }; std::vector parser_buffer; bool parser_buffer_in_use = false; @@ -101,7 +101,7 @@ class BindingData : public BaseObject { }; // TODO(addaleax): Remove once we're on C++17. -constexpr FastStringKey BindingData::binding_data_name; +constexpr FastStringKey BindingData::type_name; // helper class for the Parser struct StringPtr { diff --git a/src/node_v8.cc b/src/node_v8.cc index 4ab87dce326bb1..d66b5e03b8620c 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -95,7 +95,7 @@ class BindingData : public BaseObject { heap_code_statistics_buffer(env->isolate(), kHeapCodeStatisticsPropertiesCount) {} - static constexpr FastStringKey binding_data_name { "v8" }; + static constexpr FastStringKey type_name { "v8" }; AliasedFloat64Array heap_statistics_buffer; AliasedFloat64Array heap_space_statistics_buffer; @@ -113,7 +113,7 @@ class BindingData : public BaseObject { }; // TODO(addaleax): Remove once we're on C++17. -constexpr FastStringKey BindingData::binding_data_name; +constexpr FastStringKey BindingData::type_name; void CachedDataVersionTag(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); From 910040d5181d8375858a3a29ac5649c87ffbcecc Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 28 Jan 2021 23:37:21 +0800 Subject: [PATCH 2/2] src: refactor v8 binding 1. Put the v8 binding data class into a header so we can reuse the class definition during deserialization. 2. Put the v8 binding code into node::v8_utils namespace for clarity. 3. Move the binding data property initialization into its constructor so that we can reuse it during deserialization 4. Reorder the v8 binding initialization so that we don't unnecessarily initialize the properties in a loop --- node.gyp | 1 + src/node_v8.cc | 100 ++++++++++++++++++++----------------------------- src/node_v8.h | 36 ++++++++++++++++++ 3 files changed, 77 insertions(+), 60 deletions(-) create mode 100644 src/node_v8.h diff --git a/node.gyp b/node.gyp index 01ef5e3b6a2f27..529b4671054ca8 100644 --- a/node.gyp +++ b/node.gyp @@ -744,6 +744,7 @@ 'src/node_union_bytes.h', 'src/node_url.h', 'src/node_version.h', + 'src/node_v8.h', 'src/node_v8_platform-inl.h', 'src/node_wasi.h', 'src/node_watchdog.h', diff --git a/src/node_v8.cc b/src/node_v8.cc index d66b5e03b8620c..4354e1e1772d82 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -19,15 +19,16 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. -#include "node.h" +#include "node_v8.h" #include "base_object-inl.h" #include "env-inl.h" #include "memory_tracker-inl.h" +#include "node.h" #include "util-inl.h" #include "v8.h" namespace node { - +namespace v8_utils { using v8::Array; using v8::Context; using v8::FunctionCallbackInfo; @@ -44,7 +45,6 @@ using v8::Uint32; using v8::V8; using v8::Value; - #define HEAP_STATISTICS_PROPERTIES(V) \ V(0, total_heap_size, kTotalHeapSizeIndex) \ V(1, total_heap_size_executable, kTotalHeapSizeExecutableIndex) \ @@ -63,7 +63,6 @@ static constexpr size_t kHeapStatisticsPropertiesCount = HEAP_STATISTICS_PROPERTIES(V); #undef V - #define HEAP_SPACE_STATISTICS_PROPERTIES(V) \ V(0, space_size, kSpaceSizeIndex) \ V(1, space_used_size, kSpaceUsedSizeIndex) \ @@ -85,32 +84,34 @@ static const size_t kHeapCodeStatisticsPropertiesCount = HEAP_CODE_STATISTICS_PROPERTIES(V); #undef V -class BindingData : public BaseObject { - public: - BindingData(Environment* env, Local obj) - : BaseObject(env, obj), - heap_statistics_buffer(env->isolate(), kHeapStatisticsPropertiesCount), - heap_space_statistics_buffer(env->isolate(), - kHeapSpaceStatisticsPropertiesCount), - heap_code_statistics_buffer(env->isolate(), - kHeapCodeStatisticsPropertiesCount) {} - - static constexpr FastStringKey type_name { "v8" }; - - AliasedFloat64Array heap_statistics_buffer; - AliasedFloat64Array heap_space_statistics_buffer; - AliasedFloat64Array heap_code_statistics_buffer; - - void MemoryInfo(MemoryTracker* tracker) const override { - tracker->TrackField("heap_statistics_buffer", heap_statistics_buffer); - tracker->TrackField("heap_space_statistics_buffer", - heap_space_statistics_buffer); - tracker->TrackField("heap_code_statistics_buffer", - heap_code_statistics_buffer); - } - SET_SELF_SIZE(BindingData) - SET_MEMORY_INFO_NAME(BindingData) -}; +BindingData::BindingData(Environment* env, Local obj) + : BaseObject(env, obj), + heap_statistics_buffer(env->isolate(), kHeapStatisticsPropertiesCount), + heap_space_statistics_buffer(env->isolate(), + kHeapSpaceStatisticsPropertiesCount), + heap_code_statistics_buffer(env->isolate(), + kHeapCodeStatisticsPropertiesCount) { + obj->Set(env->context(), + FIXED_ONE_BYTE_STRING(env->isolate(), "heapStatisticsBuffer"), + heap_statistics_buffer.GetJSArray()) + .Check(); + obj->Set(env->context(), + FIXED_ONE_BYTE_STRING(env->isolate(), "heapCodeStatisticsBuffer"), + heap_code_statistics_buffer.GetJSArray()) + .Check(); + obj->Set(env->context(), + FIXED_ONE_BYTE_STRING(env->isolate(), "heapSpaceStatisticsBuffer"), + heap_space_statistics_buffer.GetJSArray()) + .Check(); +} + +void BindingData::MemoryInfo(MemoryTracker* tracker) const { + tracker->TrackField("heap_statistics_buffer", heap_statistics_buffer); + tracker->TrackField("heap_space_statistics_buffer", + heap_space_statistics_buffer); + tracker->TrackField("heap_code_statistics_buffer", + heap_code_statistics_buffer); +} // TODO(addaleax): Remove once we're on C++17. constexpr FastStringKey BindingData::type_name; @@ -179,36 +180,12 @@ void Initialize(Local target, env->SetMethodNoSideEffect(target, "cachedDataVersionTag", CachedDataVersionTag); - - // Export symbols used by v8.getHeapStatistics() env->SetMethod( target, "updateHeapStatisticsBuffer", UpdateHeapStatisticsBuffer); - target - ->Set(env->context(), - FIXED_ONE_BYTE_STRING(env->isolate(), "heapStatisticsBuffer"), - binding_data->heap_statistics_buffer.GetJSArray()) - .Check(); - -#define V(i, _, name) \ - target->Set(env->context(), \ - FIXED_ONE_BYTE_STRING(env->isolate(), #name), \ - Uint32::NewFromUnsigned(env->isolate(), i)).Check(); - - HEAP_STATISTICS_PROPERTIES(V) - - // Export symbols used by v8.getHeapCodeStatistics() env->SetMethod( target, "updateHeapCodeStatisticsBuffer", UpdateHeapCodeStatisticsBuffer); - target - ->Set(env->context(), - FIXED_ONE_BYTE_STRING(env->isolate(), "heapCodeStatisticsBuffer"), - binding_data->heap_code_statistics_buffer.GetJSArray()) - .Check(); - - HEAP_CODE_STATISTICS_PROPERTIES(V) - size_t number_of_heap_spaces = env->isolate()->NumberOfHeapSpaces(); // Heap space names are extracted once and exposed to JavaScript to @@ -230,13 +207,15 @@ void Initialize(Local target, "updateHeapSpaceStatisticsBuffer", UpdateHeapSpaceStatisticsBuffer); - target - ->Set(env->context(), - FIXED_ONE_BYTE_STRING(env->isolate(), - "heapSpaceStatisticsBuffer"), - binding_data->heap_space_statistics_buffer.GetJSArray()) +#define V(i, _, name) \ + target \ + ->Set(env->context(), \ + FIXED_ONE_BYTE_STRING(env->isolate(), #name), \ + Uint32::NewFromUnsigned(env->isolate(), i)) \ .Check(); + HEAP_STATISTICS_PROPERTIES(V) + HEAP_CODE_STATISTICS_PROPERTIES(V) HEAP_SPACE_STATISTICS_PROPERTIES(V) #undef V @@ -244,6 +223,7 @@ void Initialize(Local target, env->SetMethod(target, "setFlagsFromString", SetFlagsFromString); } +} // namespace v8_utils } // namespace node -NODE_MODULE_CONTEXT_AWARE_INTERNAL(v8, node::Initialize) +NODE_MODULE_CONTEXT_AWARE_INTERNAL(v8, node::v8_utils::Initialize) diff --git a/src/node_v8.h b/src/node_v8.h new file mode 100644 index 00000000000000..745c6580b844f4 --- /dev/null +++ b/src/node_v8.h @@ -0,0 +1,36 @@ +#ifndef SRC_NODE_V8_H_ +#define SRC_NODE_V8_H_ + +#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#include "aliased_buffer.h" +#include "base_object.h" +#include "util.h" +#include "v8.h" + +namespace node { +class Environment; + +namespace v8_utils { +class BindingData : public BaseObject { + public: + BindingData(Environment* env, v8::Local obj); + + static constexpr FastStringKey type_name{"node::v8::BindingData"}; + + AliasedFloat64Array heap_statistics_buffer; + AliasedFloat64Array heap_space_statistics_buffer; + AliasedFloat64Array heap_code_statistics_buffer; + + void MemoryInfo(MemoryTracker* tracker) const override; + SET_SELF_SIZE(BindingData) + SET_MEMORY_INFO_NAME(BindingData) +}; + +} // namespace v8_utils + +} // namespace node + +#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS + +#endif // SRC_NODE_V8_H_