Skip to content

Commit

Permalink
src: make CompiledFnEntry a BaseObject
Browse files Browse the repository at this point in the history
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: #28671

PR-URL: #28782
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
addaleax authored and Trott committed Jul 22, 2019
1 parent 3117899 commit 89e4b36
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 34 deletions.
20 changes: 0 additions & 20 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,13 @@ using v8::NewStringType;
using v8::Number;
using v8::Object;
using v8::Private;
using v8::ScriptOrModule;
using v8::SnapshotCreator;
using v8::StackTrace;
using v8::String;
using v8::Symbol;
using v8::TracingController;
using v8::Undefined;
using v8::Value;
using v8::WeakCallbackInfo;
using worker::Worker;

int const Environment::kNodeContextTag = 0x6e6f64;
Expand Down Expand Up @@ -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);
Expand Down
14 changes: 3 additions & 11 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ namespace node {

namespace contextify {
class ContextifyScript;
class CompiledFnEntry;
}

namespace fs {
Expand Down Expand Up @@ -377,6 +378,7 @@ constexpr size_t kFsStatsBufferLength =
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) \
Expand Down Expand Up @@ -523,16 +525,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) \
Expand Down Expand Up @@ -1017,7 +1009,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();
Expand Down
4 changes: 3 additions & 1 deletion src/module_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
37 changes: 35 additions & 2 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
19 changes: 19 additions & 0 deletions src/node_contextify.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 89e4b36

Please sign in to comment.