FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: make `CompiledFnEntry` a `BaseObject` · sam-github/node@89e4b36 · GitHub

/ node Public
forked from nodejs/node

Commit 89e4b36

Browse files
addaleax
authored and
Trott
committed
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: nodejs#28671 PR-URL: nodejs#28782 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent 3117899 commit 89e4b36

5 files changed

Lines changed: 60 additions & 34 deletions

File tree

‎src/env.cc‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,13 @@ using v8::NewStringType;
3939
using v8::Number;
4040
using v8::Object;
4141
using v8::Private;
42-
using v8::ScriptOrModule;
4342
using v8::SnapshotCreator;
4443
using v8::StackTrace;
4544
using v8::String;
4645
using v8::Symbol;
4746
using v8::TracingController;
4847
using v8::Undefined;
4948
using v8::Value;
50-
using v8::WeakCallbackInfo;
5149
using worker::Worker;
5250

5351
int const Environment::kNodeContextTag = 0x6e6f64;
@@ -387,24 +385,6 @@ Environment::Environment(IsolateData* isolate_data,
387385
CreateProperties();
388386
}
389387

390-
static void WeakCallbackCompiledFn(
391-
const WeakCallbackInfo<CompiledFnEntry>& data) {
392-
CompiledFnEntry* entry = data.GetParameter();
393-
entry->env->id_to_function_map.erase(entry->id);
394-
delete entry;
395-
}
396-
397-
CompiledFnEntry::CompiledFnEntry(Environment* env,
398-
uint32_t id,
399-
Local<ScriptOrModule> script)
400-
: env(env),
401-
id(id),
402-
cache_key(env->isolate(), Object::New(env->isolate())),
403-
script(env->isolate(), script) {
404-
this->script.SetWeak(
405-
this, WeakCallbackCompiledFn, v8::WeakCallbackType::kParameter);
406-
}
407-
408388
Environment::~Environment() {
409389
isolate()->GetHeapProfiler()->RemoveBuildEmbedderGraphCallback(
410390
BuildEmbedderGraph, this);

‎src/env.h‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ namespace node {
5555

5656
namespace contextify {
5757
class ContextifyScript;
58+
class CompiledFnEntry;
5859
}
5960

6061
namespace fs {
@@ -377,6 +378,7 @@ constexpr size_t kFsStatsBufferLength =
377378
V(as_callback_data_template, v8::FunctionTemplate) \
378379
V(async_wrap_ctor_template, v8::FunctionTemplate) \
379380
V(async_wrap_object_ctor_template, v8::FunctionTemplate) \
381+
V(compiled_fn_entry_template, v8::ObjectTemplate) \
380382
V(fd_constructor_template, v8::ObjectTemplate) \
381383
V(fdclose_constructor_template, v8::ObjectTemplate) \
382384
V(filehandlereadwrap_template, v8::ObjectTemplate) \
@@ -523,16 +525,6 @@ struct ContextInfo {
523525
bool is_default = false;
524526
};
525527

526-
struct CompiledFnEntry {
527-
Environment* env;
528-
uint32_t id;
529-
v8::Global<v8::Object> cache_key;
530-
v8::Global<v8::ScriptOrModule> script;
531-
CompiledFnEntry(Environment* env,
532-
uint32_t id,
533-
v8::Local<v8::ScriptOrModule> script);
534-
};
535-
536528
// Listing the AsyncWrap provider types first enables us to cast directly
537529
// from a provider type to a debug category.
538530
#define DEBUG_CATEGORY_NAMES(V) \
@@ -1017,7 +1009,7 @@ class Environment : public MemoryRetainer {
10171009
std::unordered_map<uint32_t, loader::ModuleWrap*> id_to_module_map;
10181010
std::unordered_map<uint32_t, contextify::ContextifyScript*>
10191011
id_to_script_map;
1020-
std::unordered_map<uint32_t, CompiledFnEntry*> id_to_function_map;
1012+
std::unordered_map<uint32_t, contextify::CompiledFnEntry*> id_to_function_map;
10211013

10221014
inline uint32_t get_next_module_id();
10231015
inline uint32_t get_next_script_id();

‎src/module_wrap.cc‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1041,7 +1041,9 @@ static MaybeLocal<Promise> ImportModuleDynamically(
10411041
ModuleWrap* wrap = ModuleWrap::GetFromID(env, id);
10421042
object = wrap->object();
10431043
} else if (type == ScriptType::kFunction) {
1044-
object = env->id_to_function_map.find(id)->second->cache_key.Get(iso);
1044+
auto it = env->id_to_function_map.find(id);
1045+
CHECK_NE(it, env->id_to_function_map.end());
1046+
object = it->second->object();
10451047
} else {
10461048
UNREACHABLE();
10471049
}

‎src/node_contextify.cc‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1124,9 +1124,13 @@ void ContextifyContext::CompileFunction(
11241124
}
11251125
Local<Function> fn = maybe_fn.ToLocalChecked();
11261126

1127-
CompiledFnEntry* entry = new CompiledFnEntry(env, id, script);
1127+
Local<Object> cache_key;
1128+
if (!env->compiled_fn_entry_template()->NewInstance(
1129+
context).ToLocal(&cache_key)) {
1130+
return;
1131+
}
1132+
CompiledFnEntry* entry = new CompiledFnEntry(env, cache_key, id, script);
11281133
env->id_to_function_map.emplace(id, entry);
1129-
Local<Object> cache_key = entry->cache_key.Get(isolate);
11301134

11311135
Local<Object> result = Object::New(isolate);
11321136
if (result->Set(parsing_context, env->function_string(), fn).IsNothing())
@@ -1162,6 +1166,27 @@ void ContextifyContext::CompileFunction(
11621166
args.GetReturnValue().Set(result);
11631167
}
11641168

1169+
void CompiledFnEntry::WeakCallback(
1170+
const WeakCallbackInfo<CompiledFnEntry>& data) {
1171+
CompiledFnEntry* entry = data.GetParameter();
1172+
delete entry;
1173+
}
1174+
1175+
CompiledFnEntry::CompiledFnEntry(Environment* env,
1176+
Local<Object> object,
1177+
uint32_t id,
1178+
Local<ScriptOrModule> script)
1179+
: BaseObject(env, object),
1180+
id_(id),
1181+
script_(env->isolate(), script) {
1182+
script_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
1183+
}
1184+
1185+
CompiledFnEntry::~CompiledFnEntry() {
1186+
env()->id_to_function_map.erase(id_);
1187+
script_.ClearWeak();
1188+
}
1189+
11651190
static void StartSigintWatchdog(const FunctionCallbackInfo<Value>& args) {
11661191
int ret = SigintWatchdogHelper::GetInstance()->Start();
11671192
args.GetReturnValue().Set(ret == 0);
@@ -1190,6 +1215,14 @@ void Initialize(Local<Object> target,
11901215
// Used in tests.
11911216
env->SetMethodNoSideEffect(
11921217
target, "watchdogHasPendingSigint", WatchdogHasPendingSigint);
1218+
1219+
{
1220+
Local<FunctionTemplate> tpl = FunctionTemplate::New(env->isolate());
1221+
tpl->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "CompiledFnEntry"));
1222+
tpl->InstanceTemplate()->SetInternalFieldCount(1);
1223+
1224+
env->set_compiled_fn_entry_template(tpl->InstanceTemplate());
1225+
}
11931226
}
11941227

11951228
} // namespace contextify

‎src/node_contextify.h‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,25 @@ class ContextifyScript : public BaseObject {
133133
uint32_t id_;
134134
};
135135

136+
class CompiledFnEntry final : public BaseObject {
137+
public:
138+
SET_NO_MEMORY_INFO()
139+
SET_MEMORY_INFO_NAME(CompiledFnEntry)
140+
SET_SELF_SIZE(CompiledFnEntry)
141+
142+
CompiledFnEntry(Environment* env,
143+
v8::Local<v8::Object> object,
144+
uint32_t id,
145+
v8::Local<v8::ScriptOrModule> script);
146+
~CompiledFnEntry();
147+
148+
private:
149+
uint32_t id_;
150+
v8::Global<v8::ScriptOrModule> script_;
151+
152+
static void WeakCallback(const v8::WeakCallbackInfo<CompiledFnEntry>& data);
153+
};
154+
136155
} // namespace contextify
137156
} // namespace node
138157

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL