diff options
author | Anna Henningsen <anna@addaleax.net> | 2019-07-20 19:35:24 +0200 |
---|---|---|
committer | Michaël Zasso <targos@protonmail.com> | 2019-07-23 08:15:41 +0200 |
commit | c0f24be185a995ae05e55c44fde0ff83e81e63d9 (patch) | |
tree | 340be70ce687cc56721e54c8d6feb88c31cfeff7 | |
parent | 7b4638cee03771d9666ad0ba0c494046fe84dd8d (diff) | |
download | android-node-v8-c0f24be185a995ae05e55c44fde0ff83e81e63d9.tar.gz android-node-v8-c0f24be185a995ae05e55c44fde0ff83e81e63d9.tar.bz2 android-node-v8-c0f24be185a995ae05e55c44fde0ff83e81e63d9.zip |
src: make `CompiledFnEntry` a `BaseObject`
In particular:
- Move the class definition to the relevant header file,
i.e. `node_contextify.h`.
- Make sure that class instances are destroyed on
`Environment` teardown.
- Make instances of the key object traceable in heap dumps. This is
particularly relevant here because our C++ script → map key mapping
could introduce memory leaks when the import function metadata refers
back to the script in some way.
Refs: https://github.com/nodejs/node/pull/28671
PR-URL: https://github.com/nodejs/node/pull/28782
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
-rw-r--r-- | src/env.cc | 20 | ||||
-rw-r--r-- | src/env.h | 14 | ||||
-rw-r--r-- | src/module_wrap.cc | 4 | ||||
-rw-r--r-- | src/node_contextify.cc | 37 | ||||
-rw-r--r-- | src/node_contextify.h | 19 |
5 files changed, 60 insertions, 34 deletions
diff --git a/src/env.cc b/src/env.cc index 38d5796f54..bb78a0c1f3 100644 --- a/src/env.cc +++ b/src/env.cc @@ -39,7 +39,6 @@ using v8::NewStringType; using v8::Number; using v8::Object; using v8::Private; -using v8::ScriptOrModule; using v8::SnapshotCreator; using v8::StackTrace; using v8::String; @@ -47,7 +46,6 @@ using v8::Symbol; using v8::TracingController; using v8::Undefined; using v8::Value; -using v8::WeakCallbackInfo; using worker::Worker; int const Environment::kNodeContextTag = 0x6e6f64; @@ -387,24 +385,6 @@ Environment::Environment(IsolateData* isolate_data, CreateProperties(); } -static void WeakCallbackCompiledFn( - const WeakCallbackInfo<CompiledFnEntry>& data) { - CompiledFnEntry* entry = data.GetParameter(); - entry->env->id_to_function_map.erase(entry->id); - delete entry; -} - -CompiledFnEntry::CompiledFnEntry(Environment* env, - uint32_t id, - Local<ScriptOrModule> script) - : env(env), - id(id), - cache_key(env->isolate(), Object::New(env->isolate())), - script(env->isolate(), script) { - this->script.SetWeak( - this, WeakCallbackCompiledFn, v8::WeakCallbackType::kParameter); -} - Environment::~Environment() { isolate()->GetHeapProfiler()->RemoveBuildEmbedderGraphCallback( BuildEmbedderGraph, this); @@ -55,6 +55,7 @@ namespace node { namespace contextify { class ContextifyScript; +class CompiledFnEntry; } namespace fs { @@ -355,6 +356,7 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2; V(as_callback_data_template, v8::FunctionTemplate) \ V(async_wrap_ctor_template, v8::FunctionTemplate) \ V(async_wrap_object_ctor_template, v8::FunctionTemplate) \ + V(compiled_fn_entry_template, v8::ObjectTemplate) \ V(fd_constructor_template, v8::ObjectTemplate) \ V(fdclose_constructor_template, v8::ObjectTemplate) \ V(filehandlereadwrap_template, v8::ObjectTemplate) \ @@ -501,16 +503,6 @@ struct ContextInfo { bool is_default = false; }; -struct CompiledFnEntry { - Environment* env; - uint32_t id; - v8::Global<v8::Object> cache_key; - v8::Global<v8::ScriptOrModule> script; - CompiledFnEntry(Environment* env, - uint32_t id, - v8::Local<v8::ScriptOrModule> script); -}; - // Listing the AsyncWrap provider types first enables us to cast directly // from a provider type to a debug category. #define DEBUG_CATEGORY_NAMES(V) \ @@ -995,7 +987,7 @@ class Environment : public MemoryRetainer { std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map; std::unordered_map<uint32_t, contextify::ContextifyScript*> id_to_script_map; - std::unordered_map<uint32_t, CompiledFnEntry*> id_to_function_map; + std::unordered_map<uint32_t, contextify::CompiledFnEntry*> id_to_function_map; inline uint32_t get_next_module_id(); inline uint32_t get_next_script_id(); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 7df91bb870..c13ab3b5ed 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -1041,7 +1041,9 @@ static MaybeLocal<Promise> ImportModuleDynamically( ModuleWrap* wrap = ModuleWrap::GetFromID(env, id); object = wrap->object(); } else if (type == ScriptType::kFunction) { - object = env->id_to_function_map.find(id)->second->cache_key.Get(iso); + auto it = env->id_to_function_map.find(id); + CHECK_NE(it, env->id_to_function_map.end()); + object = it->second->object(); } else { UNREACHABLE(); } diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 6559e813c9..69c110d6be 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1124,9 +1124,13 @@ void ContextifyContext::CompileFunction( } Local<Function> fn = maybe_fn.ToLocalChecked(); - CompiledFnEntry* entry = new CompiledFnEntry(env, id, script); + Local<Object> cache_key; + if (!env->compiled_fn_entry_template()->NewInstance( + context).ToLocal(&cache_key)) { + return; + } + CompiledFnEntry* entry = new CompiledFnEntry(env, cache_key, id, script); env->id_to_function_map.emplace(id, entry); - Local<Object> cache_key = entry->cache_key.Get(isolate); Local<Object> result = Object::New(isolate); if (result->Set(parsing_context, env->function_string(), fn).IsNothing()) @@ -1162,6 +1166,27 @@ void ContextifyContext::CompileFunction( args.GetReturnValue().Set(result); } +void CompiledFnEntry::WeakCallback( + const WeakCallbackInfo<CompiledFnEntry>& data) { + CompiledFnEntry* entry = data.GetParameter(); + delete entry; +} + +CompiledFnEntry::CompiledFnEntry(Environment* env, + Local<Object> object, + uint32_t id, + Local<ScriptOrModule> script) + : BaseObject(env, object), + id_(id), + script_(env->isolate(), script) { + script_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); +} + +CompiledFnEntry::~CompiledFnEntry() { + env()->id_to_function_map.erase(id_); + script_.ClearWeak(); +} + static void StartSigintWatchdog(const FunctionCallbackInfo<Value>& args) { int ret = SigintWatchdogHelper::GetInstance()->Start(); args.GetReturnValue().Set(ret == 0); @@ -1190,6 +1215,14 @@ void Initialize(Local<Object> target, // Used in tests. env->SetMethodNoSideEffect( target, "watchdogHasPendingSigint", WatchdogHasPendingSigint); + + { + Local<FunctionTemplate> tpl = FunctionTemplate::New(env->isolate()); + tpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "CompiledFnEntry")); + tpl->InstanceTemplate()->SetInternalFieldCount(1); + + env->set_compiled_fn_entry_template(tpl->InstanceTemplate()); + } } } // namespace contextify diff --git a/src/node_contextify.h b/src/node_contextify.h index 73461dc623..cf1e847507 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -133,6 +133,25 @@ class ContextifyScript : public BaseObject { uint32_t id_; }; +class CompiledFnEntry final : public BaseObject { + public: + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(CompiledFnEntry) + SET_SELF_SIZE(CompiledFnEntry) + + CompiledFnEntry(Environment* env, + v8::Local<v8::Object> object, + uint32_t id, + v8::Local<v8::ScriptOrModule> script); + ~CompiledFnEntry(); + + private: + uint32_t id_; + v8::Global<v8::ScriptOrModule> script_; + + static void WeakCallback(const v8::WeakCallbackInfo<CompiledFnEntry>& data); +}; + } // namespace contextify } // namespace node |