Skip to content

Commit

Permalink
src: prefer v8::Global over node::Persistent
Browse files Browse the repository at this point in the history
`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: #27287
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
  • Loading branch information
addaleax authored and targos committed Apr 30, 2019
1 parent eeab007 commit 3370291
Show file tree
Hide file tree
Showing 23 changed files with 81 additions and 106 deletions.
1 change: 0 additions & 1 deletion node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
5 changes: 3 additions & 2 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -346,8 +347,8 @@ class DestroyParam {
public:
double asyncId;
Environment* env;
Persistent<Object> target;
Persistent<Object> propBag;
Global<Object> target;
Global<Object> propBag;
};

void AsyncWrap::WeakCallback(const WeakCallbackInfo<DestroyParam>& info) {
Expand Down
2 changes: 1 addition & 1 deletion src/base_object-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ BaseObject::~BaseObject() {
}


Persistent<v8::Object>& BaseObject::persistent() {
v8::Global<v8::Object>& BaseObject::persistent() {
return persistent_handle_;
}

Expand Down
7 changes: 3 additions & 4 deletions src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 <type_traits> // std::remove_reference
Expand All @@ -50,7 +49,7 @@ class BaseObject : public MemoryRetainer {
// is associated with the passed Isolate in debug mode.
inline v8::Local<v8::Object> object(v8::Isolate* isolate) const;

inline Persistent<v8::Object>& persistent();
inline v8::Global<v8::Object>& persistent();

inline Environment* env() const;

Expand All @@ -62,7 +61,7 @@ class BaseObject : public MemoryRetainer {
template <typename T>
static inline T* FromJSObject(v8::Local<v8::Object> 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();

Expand Down Expand Up @@ -96,7 +95,7 @@ class BaseObject : public MemoryRetainer {
friend int GenDebugSymbols();
friend class CleanupHookCallback;

Persistent<v8::Object> persistent_handle_;
v8::Global<v8::Object> persistent_handle_;
Environment* env_;
};

Expand Down
4 changes: 2 additions & 2 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -929,7 +929,7 @@ class Environment : public MemoryRetainer {
std::unordered_map<uint32_t, contextify::ContextifyScript*>
id_to_script_map;
std::unordered_set<CompileFnEntry*> compile_fn_entries;
std::unordered_map<uint32_t, Persistent<v8::Function>> id_to_function_map;
std::unordered_map<uint32_t, v8::Global<v8::Function>> id_to_function_map;

inline uint32_t get_next_module_id();
inline uint32_t get_next_script_id();
Expand Down Expand Up @@ -1292,7 +1292,7 @@ class Environment : public MemoryRetainer {
template <typename T>
void ForEachBaseObject(T&& iterator);

#define V(PropertyName, TypeName) Persistent<TypeName> PropertyName ## _;
#define V(PropertyName, TypeName) v8::Global<TypeName> PropertyName ## _;
ENVIRONMENT_STRONG_PERSISTENT_VALUES(V)
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
#undef V
Expand Down
3 changes: 2 additions & 1 deletion src/heap_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -56,7 +57,7 @@ class JSGraphJSNode : public EmbedderGraph::Node {
};

private:
Persistent<Value> persistent_;
Global<Value> persistent_;
};

class JSGraph : public EmbedderGraph {
Expand Down
3 changes: 2 additions & 1 deletion src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -834,7 +835,7 @@ void Agent::DisableAsyncHook() {
}

void Agent::ToggleAsyncHook(Isolate* isolate,
const node::Persistent<Function>& fn) {
const Global<Function>& fn) {
CHECK(parent_env_->has_run_bootstrapping_code());
HandleScope handle_scope(isolate);
CHECK(!fn.IsEmpty());
Expand Down
7 changes: 3 additions & 4 deletions src/inspector_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#endif

#include "node_options-inl.h"
#include "node_persistent.h"
#include "v8.h"

#include <cstddef>
Expand Down Expand Up @@ -114,7 +113,7 @@ class Agent {

private:
void ToggleAsyncHook(v8::Isolate* isolate,
const node::Persistent<v8::Function>& fn);
const v8::Global<v8::Function>& fn);

node::Environment* parent_env_;
// Encapsulates majority of the Inspector functionality
Expand All @@ -133,8 +132,8 @@ class Agent {

bool pending_enable_async_hook_ = false;
bool pending_disable_async_hook_ = false;
node::Persistent<v8::Function> enable_async_hook_function_;
node::Persistent<v8::Function> disable_async_hook_function_;
v8::Global<v8::Function> enable_async_hook_function_;
v8::Global<v8::Function> disable_async_hook_function_;
};

} // namespace inspector
Expand Down
3 changes: 2 additions & 1 deletion src/inspector_js_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -116,7 +117,7 @@ class JSBindingsConnection : public AsyncWrap {

private:
std::unique_ptr<InspectorSession> session_;
Persistent<Function> callback_;
Global<Function> callback_;
};

static bool InspectorEnabled(Environment* env) {
Expand Down
2 changes: 1 addition & 1 deletion src/js_native_api_v8_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
namespace v8impl {

template <typename T>
using Persistent = node::Persistent<T>;
using Persistent = v8::Global<T>;

using PersistentToLocal = node::PersistentToLocal;

Expand Down
4 changes: 2 additions & 2 deletions src/memory_tracker-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,9 @@ void MemoryTracker::TrackField(const char* edge_name,
TrackField(edge_name, value.Get(isolate_));
}

template <typename T, typename Traits>
template <typename T>
void MemoryTracker::TrackField(const char* edge_name,
const v8::Persistent<T, Traits>& value,
const v8::PersistentBase<T>& value,
const char* node_name) {
TrackField(edge_name, value.Get(isolate_));
}
Expand Down
8 changes: 4 additions & 4 deletions src/memory_tracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,9 @@ class CleanupHookCallback;
* NonPointerRetainerClass non_pointer_retainer;
* InternalClass internal_member_;
* std::vector<uv_async_t> vector_;
* node::Persistent<Object> target_;
* v8::Global<Object> target_;
*
* node::Persistent<Object> wrapped_;
* v8::Global<Object> wrapped_;
* }
*
* This creates the following graph:
Expand Down Expand Up @@ -185,9 +185,9 @@ class MemoryTracker {
void TrackField(const char* edge_name,
const v8::Eternal<T>& value,
const char* node_name);
template <typename T, typename Traits>
template <typename T>
inline void TrackField(const char* edge_name,
const v8::Persistent<T, Traits>& value,
const v8::PersistentBase<T>& value,
const char* node_name = nullptr);
template <typename T>
inline void TrackField(const char* edge_name,
Expand Down
3 changes: 2 additions & 1 deletion src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -611,7 +612,7 @@ Maybe<const PackageConfig*> GetPackageConfig(Environment* env,
env->exports_string()).ToLocal(&exports_v) &&
(exports_v->IsObject() || exports_v->IsString() ||
exports_v->IsBoolean())) {
Persistent<Value> exports;
Global<Value> exports;
exports.Reset(env->isolate(), exports_v);

auto entry = env->package_json_cache.emplace(path,
Expand Down
8 changes: 4 additions & 4 deletions src/module_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,11 @@ class ModuleWrap : public BaseObject {
v8::Local<v8::Module> referrer);
static ModuleWrap* GetFromModule(node::Environment*, v8::Local<v8::Module>);

Persistent<v8::Module> module_;
Persistent<v8::String> url_;
v8::Global<v8::Module> module_;
v8::Global<v8::String> url_;
bool linked_ = false;
std::unordered_map<std::string, Persistent<v8::Promise>> resolve_cache_;
Persistent<v8::Context> context_;
std::unordered_map<std::string, v8::Global<v8::Promise>> resolve_cache_;
v8::Global<v8::Context> context_;
uint32_t id_;
};

Expand Down
3 changes: 2 additions & 1 deletion src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -99,7 +100,7 @@ class CallbackInfo {
FreeCallback callback,
char* data,
void* hint);
Persistent<ArrayBuffer> persistent_;
Global<ArrayBuffer> persistent_;
FreeCallback const callback_;
char* const data_;
void* const hint_;
Expand Down
4 changes: 2 additions & 2 deletions src/node_contextify.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class ContextifyContext {
uint32_t index,
const v8::PropertyCallbackInfo<v8::Boolean>& args);
Environment* const env_;
Persistent<v8::Context> context_;
v8::Global<v8::Context> context_;
};

class ContextifyScript : public BaseObject {
Expand Down Expand Up @@ -129,7 +129,7 @@ class ContextifyScript : public BaseObject {
inline uint32_t id() { return id_; }

private:
node::Persistent<v8::UnboundScript> script_;
v8::Global<v8::UnboundScript> script_;
uint32_t id_;
};

Expand Down
4 changes: 2 additions & 2 deletions src/node_crypto.h
Original file line number Diff line number Diff line change
Expand Up @@ -321,8 +321,8 @@ class SSLWrap {

ClientHelloParser hello_parser_;

Persistent<v8::ArrayBufferView> ocsp_response_;
Persistent<v8::Value> sni_context_;
v8::Global<v8::ArrayBufferView> ocsp_response_;
v8::Global<v8::Value> sni_context_;

friend class SecureContext;
};
Expand Down
4 changes: 2 additions & 2 deletions src/node_file.h
Original file line number Diff line number Diff line change
Expand Up @@ -451,8 +451,8 @@ class FileHandle : public AsyncWrap, public StreamBase {
CloseReq& operator=(const CloseReq&&) = delete;

private:
Persistent<Promise> promise_{};
Persistent<Value> ref_{};
v8::Global<Promise> promise_{};
v8::Global<Value> ref_{};
};

// Asynchronous close
Expand Down
1 change: 0 additions & 1 deletion src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
66 changes: 0 additions & 66 deletions src/node_persistent.h

This file was deleted.

3 changes: 2 additions & 1 deletion src/node_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -189,7 +190,7 @@ class WeakReference : public BaseObject {
SET_NO_MEMORY_INFO()

private:
Persistent<Object> target_;
Global<Object> target_;
};

static void GuessHandleType(const FunctionCallbackInfo<Value>& args) {
Expand Down
Loading

0 comments on commit 3370291

Please sign in to comment.