From 33702913b1136607b1f0859375f3d2fa939fbb29 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 17 Apr 2019 23:16:22 +0200 Subject: [PATCH] src: prefer v8::Global over node::Persistent `v8::Global` is essentially a nicer variant of `node::Persistent` that, in addition to reset-on-destroy, also implements move semantics. This commit makes the necessary replacements, removes `node::Persistent` and (now-)unnecessary inclusions of the `node_persistent.h` header, and makes some of the functions that take Persistents as arguments more generic so that they work with all `v8::PersistentBase` flavours. PR-URL: https://github.com/nodejs/node/pull/27287 Reviewed-By: Gus Caplan Reviewed-By: Eugene Ostroukhov Reviewed-By: Joyee Cheung Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig --- node.gyp | 1 - src/async_wrap.cc | 5 ++- src/base_object-inl.h | 2 +- src/base_object.h | 7 ++-- src/env.h | 4 +- src/heap_utils.cc | 3 +- src/inspector_agent.cc | 3 +- src/inspector_agent.h | 7 ++-- src/inspector_js_api.cc | 3 +- src/js_native_api_v8_internals.h | 2 +- src/memory_tracker-inl.h | 4 +- src/memory_tracker.h | 8 ++-- src/module_wrap.cc | 3 +- src/module_wrap.h | 8 ++-- src/node_buffer.cc | 3 +- src/node_contextify.h | 4 +- src/node_crypto.h | 4 +- src/node_file.h | 4 +- src/node_internals.h | 1 - src/node_persistent.h | 66 -------------------------------- src/node_util.cc | 3 +- src/node_zlib.cc | 3 +- src/util.h | 39 ++++++++++++++++++- 23 files changed, 81 insertions(+), 106 deletions(-) delete mode 100644 src/node_persistent.h diff --git a/node.gyp b/node.gyp index 3cf47ffc255531..daae6afb5892a9 100644 --- a/node.gyp +++ b/node.gyp @@ -606,7 +606,6 @@ 'src/node_options-inl.h', 'src/node_perf.h', 'src/node_perf_common.h', - 'src/node_persistent.h', 'src/node_platform.h', 'src/node_process.h', 'src/node_revert.h', diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 9cfa0924c79568..e82225526503a5 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -35,6 +35,7 @@ using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::Global; using v8::HandleScope; using v8::Integer; using v8::Isolate; @@ -346,8 +347,8 @@ class DestroyParam { public: double asyncId; Environment* env; - Persistent target; - Persistent propBag; + Global target; + Global propBag; }; void AsyncWrap::WeakCallback(const WeakCallbackInfo& info) { diff --git a/src/base_object-inl.h b/src/base_object-inl.h index e3d645056a5dc2..fd61c15feaee91 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -56,7 +56,7 @@ BaseObject::~BaseObject() { } -Persistent& BaseObject::persistent() { +v8::Global& BaseObject::persistent() { return persistent_handle_; } diff --git a/src/base_object.h b/src/base_object.h index f1c666224f0401..cb83462ff51e54 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -24,7 +24,6 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#include "node_persistent.h" #include "memory_tracker-inl.h" #include "v8.h" #include // std::remove_reference @@ -50,7 +49,7 @@ class BaseObject : public MemoryRetainer { // is associated with the passed Isolate in debug mode. inline v8::Local object(v8::Isolate* isolate) const; - inline Persistent& persistent(); + inline v8::Global& persistent(); inline Environment* env() const; @@ -62,7 +61,7 @@ class BaseObject : public MemoryRetainer { template static inline T* FromJSObject(v8::Local object); - // Make the `Persistent` a weak reference and, `delete` this object once + // Make the `v8::Global` a weak reference and, `delete` this object once // the JS object has been garbage collected. inline void MakeWeak(); @@ -96,7 +95,7 @@ class BaseObject : public MemoryRetainer { friend int GenDebugSymbols(); friend class CleanupHookCallback; - Persistent persistent_handle_; + v8::Global persistent_handle_; Environment* env_; }; diff --git a/src/env.h b/src/env.h index d4991535186d51..56abe129abf45e 100644 --- a/src/env.h +++ b/src/env.h @@ -929,7 +929,7 @@ class Environment : public MemoryRetainer { std::unordered_map id_to_script_map; std::unordered_set compile_fn_entries; - std::unordered_map> id_to_function_map; + std::unordered_map> id_to_function_map; inline uint32_t get_next_module_id(); inline uint32_t get_next_script_id(); @@ -1292,7 +1292,7 @@ class Environment : public MemoryRetainer { template void ForEachBaseObject(T&& iterator); -#define V(PropertyName, TypeName) Persistent PropertyName ## _; +#define V(PropertyName, TypeName) v8::Global PropertyName ## _; ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V diff --git a/src/heap_utils.cc b/src/heap_utils.cc index d5b5afa4d5be9e..654dfabafe62b6 100644 --- a/src/heap_utils.cc +++ b/src/heap_utils.cc @@ -8,6 +8,7 @@ using v8::EmbedderGraph; using v8::EscapableHandleScope; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::Global; using v8::HandleScope; using v8::HeapSnapshot; using v8::Isolate; @@ -56,7 +57,7 @@ class JSGraphJSNode : public EmbedderGraph::Node { }; private: - Persistent persistent_; + Global persistent_; }; class JSGraph : public EmbedderGraph { diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 10b607ff6c53c2..78af5508d0780b 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -34,6 +34,7 @@ using node::FatalError; using v8::Context; using v8::Function; +using v8::Global; using v8::HandleScope; using v8::Isolate; using v8::Local; @@ -834,7 +835,7 @@ void Agent::DisableAsyncHook() { } void Agent::ToggleAsyncHook(Isolate* isolate, - const node::Persistent& fn) { + const Global& fn) { CHECK(parent_env_->has_run_bootstrapping_code()); HandleScope handle_scope(isolate); CHECK(!fn.IsEmpty()); diff --git a/src/inspector_agent.h b/src/inspector_agent.h index b079ea675fbafa..c7c92186d114a3 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -7,7 +7,6 @@ #endif #include "node_options-inl.h" -#include "node_persistent.h" #include "v8.h" #include @@ -114,7 +113,7 @@ class Agent { private: void ToggleAsyncHook(v8::Isolate* isolate, - const node::Persistent& fn); + const v8::Global& fn); node::Environment* parent_env_; // Encapsulates majority of the Inspector functionality @@ -133,8 +132,8 @@ class Agent { bool pending_enable_async_hook_ = false; bool pending_disable_async_hook_ = false; - node::Persistent enable_async_hook_function_; - node::Persistent disable_async_hook_function_; + v8::Global enable_async_hook_function_; + v8::Global disable_async_hook_function_; }; } // namespace inspector diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 51ec14a63a1ad1..4948bd8797a9e4 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -15,6 +15,7 @@ using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::Global; using v8::HandleScope; using v8::Isolate; using v8::Local; @@ -116,7 +117,7 @@ class JSBindingsConnection : public AsyncWrap { private: std::unique_ptr session_; - Persistent callback_; + Global callback_; }; static bool InspectorEnabled(Environment* env) { diff --git a/src/js_native_api_v8_internals.h b/src/js_native_api_v8_internals.h index dcdc62297f6f59..ddd219818cdfa9 100644 --- a/src/js_native_api_v8_internals.h +++ b/src/js_native_api_v8_internals.h @@ -29,7 +29,7 @@ namespace v8impl { template -using Persistent = node::Persistent; +using Persistent = v8::Global; using PersistentToLocal = node::PersistentToLocal; diff --git a/src/memory_tracker-inl.h b/src/memory_tracker-inl.h index b17b8fbab2cada..da37f72c737607 100644 --- a/src/memory_tracker-inl.h +++ b/src/memory_tracker-inl.h @@ -184,9 +184,9 @@ void MemoryTracker::TrackField(const char* edge_name, TrackField(edge_name, value.Get(isolate_)); } -template +template void MemoryTracker::TrackField(const char* edge_name, - const v8::Persistent& value, + const v8::PersistentBase& value, const char* node_name) { TrackField(edge_name, value.Get(isolate_)); } diff --git a/src/memory_tracker.h b/src/memory_tracker.h index 032c9a984e51d6..d22116918afec8 100644 --- a/src/memory_tracker.h +++ b/src/memory_tracker.h @@ -90,9 +90,9 @@ class CleanupHookCallback; * NonPointerRetainerClass non_pointer_retainer; * InternalClass internal_member_; * std::vector vector_; - * node::Persistent target_; + * v8::Global target_; * - * node::Persistent wrapped_; + * v8::Global wrapped_; * } * * This creates the following graph: @@ -185,9 +185,9 @@ class MemoryTracker { void TrackField(const char* edge_name, const v8::Eternal& value, const char* node_name); - template + template inline void TrackField(const char* edge_name, - const v8::Persistent& value, + const v8::PersistentBase& value, const char* node_name = nullptr); template inline void TrackField(const char* edge_name, diff --git a/src/module_wrap.cc b/src/module_wrap.cc index d6ad0da622476f..a4e81dcc29474b 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -25,6 +25,7 @@ using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::Global; using v8::HandleScope; using v8::Integer; using v8::IntegrityLevel; @@ -611,7 +612,7 @@ Maybe GetPackageConfig(Environment* env, env->exports_string()).ToLocal(&exports_v) && (exports_v->IsObject() || exports_v->IsString() || exports_v->IsBoolean())) { - Persistent exports; + Global exports; exports.Reset(env->isolate(), exports_v); auto entry = env->package_json_cache.emplace(path, diff --git a/src/module_wrap.h b/src/module_wrap.h index fddb852a79dd70..6c79781a9d3a78 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -75,11 +75,11 @@ class ModuleWrap : public BaseObject { v8::Local referrer); static ModuleWrap* GetFromModule(node::Environment*, v8::Local); - Persistent module_; - Persistent url_; + v8::Global module_; + v8::Global url_; bool linked_ = false; - std::unordered_map> resolve_cache_; - Persistent context_; + std::unordered_map> resolve_cache_; + v8::Global context_; uint32_t id_; }; diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 58175a8fd5f615..3b4be5a8105f62 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -62,6 +62,7 @@ using v8::ArrayBufferView; using v8::Context; using v8::EscapableHandleScope; using v8::FunctionCallbackInfo; +using v8::Global; using v8::Integer; using v8::Isolate; using v8::Just; @@ -99,7 +100,7 @@ class CallbackInfo { FreeCallback callback, char* data, void* hint); - Persistent persistent_; + Global persistent_; FreeCallback const callback_; char* const data_; void* const hint_; diff --git a/src/node_contextify.h b/src/node_contextify.h index 4186e5190f8ef9..d04bf9ea28efb2 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -100,7 +100,7 @@ class ContextifyContext { uint32_t index, const v8::PropertyCallbackInfo& args); Environment* const env_; - Persistent context_; + v8::Global context_; }; class ContextifyScript : public BaseObject { @@ -129,7 +129,7 @@ class ContextifyScript : public BaseObject { inline uint32_t id() { return id_; } private: - node::Persistent script_; + v8::Global script_; uint32_t id_; }; diff --git a/src/node_crypto.h b/src/node_crypto.h index 657789afc42e1f..44206b58ddd6bf 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -321,8 +321,8 @@ class SSLWrap { ClientHelloParser hello_parser_; - Persistent ocsp_response_; - Persistent sni_context_; + v8::Global ocsp_response_; + v8::Global sni_context_; friend class SecureContext; }; diff --git a/src/node_file.h b/src/node_file.h index f5c523fb0708cc..b417d4916f8c35 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -451,8 +451,8 @@ class FileHandle : public AsyncWrap, public StreamBase { CloseReq& operator=(const CloseReq&&) = delete; private: - Persistent promise_{}; - Persistent ref_{}; + v8::Global promise_{}; + v8::Global ref_{}; }; // Asynchronous close diff --git a/src/node_internals.h b/src/node_internals.h index fc924e3e1637ce..198132d49d6429 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -28,7 +28,6 @@ #include "node.h" #include "node_binding.h" #include "node_mutex.h" -#include "node_persistent.h" #include "tracing/trace_event.h" #include "util-inl.h" #include "uv.h" diff --git a/src/node_persistent.h b/src/node_persistent.h deleted file mode 100644 index 6126ebc6c20fd5..00000000000000 --- a/src/node_persistent.h +++ /dev/null @@ -1,66 +0,0 @@ -#ifndef SRC_NODE_PERSISTENT_H_ -#define SRC_NODE_PERSISTENT_H_ - -#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS - -#include "v8.h" - -namespace node { - -template -struct ResetInDestructorPersistentTraits { - static const bool kResetInDestructor = true; - template - // Disallow copy semantics by leaving this unimplemented. - inline static void Copy( - const v8::Persistent&, - v8::Persistent>*); -}; - -// v8::Persistent does not reset the object slot in its destructor. That is -// acknowledged as a flaw in the V8 API and expected to change in the future -// but for now node::Persistent is the easier and safer alternative. -template -using Persistent = v8::Persistent>; - -class PersistentToLocal { - public: - // If persistent.IsWeak() == false, then do not call persistent.Reset() - // while the returned Local is still in scope, it will destroy the - // reference to the object. - template - static inline v8::Local Default( - v8::Isolate* isolate, - const Persistent& persistent) { - if (persistent.IsWeak()) { - return PersistentToLocal::Weak(isolate, persistent); - } else { - return PersistentToLocal::Strong(persistent); - } - } - - // Unchecked conversion from a non-weak Persistent to Local, - // use with care! - // - // Do not call persistent.Reset() while the returned Local is still in - // scope, it will destroy the reference to the object. - template - static inline v8::Local Strong( - const Persistent& persistent) { - return *reinterpret_cast*>( - const_cast*>(&persistent)); - } - - template - static inline v8::Local Weak( - v8::Isolate* isolate, - const Persistent& persistent) { - return v8::Local::New(isolate, persistent); - } -}; - -} // namespace node - -#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS - -#endif // SRC_NODE_PERSISTENT_H_ diff --git a/src/node_util.cc b/src/node_util.cc index 961df0b73c3deb..180c58c4168e1d 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -13,6 +13,7 @@ using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::Global; using v8::IndexFilter; using v8::Integer; using v8::Isolate; @@ -189,7 +190,7 @@ class WeakReference : public BaseObject { SET_NO_MEMORY_INFO() private: - Persistent target_; + Global target_; }; static void GuessHandleType(const FunctionCallbackInfo& args) { diff --git a/src/node_zlib.cc b/src/node_zlib.cc index b5abcfa5850833..f389257882215e 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -47,6 +47,7 @@ using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::Global; using v8::HandleScope; using v8::Int32; using v8::Integer; @@ -526,7 +527,7 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork { bool closed_ = false; unsigned int refs_ = 0; uint32_t* write_result_ = nullptr; - Persistent write_js_callback_; + Global write_js_callback_; std::atomic unreported_allocations_{0}; size_t zlib_memory_ = 0; diff --git a/src/util.h b/src/util.h index 3a6cef2e3524a4..c9c605685070ff 100644 --- a/src/util.h +++ b/src/util.h @@ -24,7 +24,6 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#include "node_persistent.h" #include "v8.h" #include @@ -669,6 +668,44 @@ class SlicedArguments : public MaybeStackBuffer> { const v8::FunctionCallbackInfo& args, size_t start = 0); }; +// Convert a v8::PersistentBase, e.g. v8::Global, to a Local, with an extra +// optimization for strong persistent handles. +class PersistentToLocal { + public: + // If persistent.IsWeak() == false, then do not call persistent.Reset() + // while the returned Local is still in scope, it will destroy the + // reference to the object. + template + static inline v8::Local Default( + v8::Isolate* isolate, + const v8::PersistentBase& persistent) { + if (persistent.IsWeak()) { + return PersistentToLocal::Weak(isolate, persistent); + } else { + return PersistentToLocal::Strong(persistent); + } + } + + // Unchecked conversion from a non-weak Persistent to Local, + // use with care! + // + // Do not call persistent.Reset() while the returned Local is still in + // scope, it will destroy the reference to the object. + template + static inline v8::Local Strong( + const v8::PersistentBase& persistent) { + return *reinterpret_cast*>( + const_cast*>(&persistent)); + } + + template + static inline v8::Local Weak( + v8::Isolate* isolate, + const v8::PersistentBase& persistent) { + return v8::Local::New(isolate, persistent); + } +}; + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS