Skip to content

Commit b12211e

Browse files
addaleaxMylesBorins
authored andcommitted
src: prefer internal fields in ModuleWrap
Use internal fields instead of `v8::Global`s where possible, since they generally come with lower overhead and it’s much harder to introduce memory leaks with them. PR-URL: #34470 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
1 parent 9b91467 commit b12211e

File tree

2 files changed

+33
-13
lines changed

2 files changed

+33
-13
lines changed

src/module_wrap.cc

+24-9
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,13 @@ ModuleWrap::ModuleWrap(Environment* env,
5555
Local<String> url)
5656
: BaseObject(env, object),
5757
module_(env->isolate(), module),
58-
url_(env->isolate(), url),
5958
id_(env->get_next_module_id()) {
6059
env->id_to_module_map.emplace(id_, this);
60+
61+
Local<Value> undefined = Undefined(env->isolate());
62+
object->SetInternalField(kURLSlot, url);
63+
object->SetInternalField(kSyntheticEvaluationStepsSlot, undefined);
64+
object->SetInternalField(kContextObjectSlot, undefined);
6165
}
6266

6367
ModuleWrap::~ModuleWrap() {
@@ -73,6 +77,12 @@ ModuleWrap::~ModuleWrap() {
7377
}
7478
}
7579

80+
Local<Context> ModuleWrap::context() const {
81+
Local<Value> obj = object()->GetInternalField(kContextObjectSlot);
82+
if (obj.IsEmpty()) return {};
83+
return obj.As<Object>()->CreationContext();
84+
}
85+
7686
ModuleWrap* ModuleWrap::GetFromModule(Environment* env,
7787
Local<Module> module) {
7888
auto range = env->hash_to_module_map.equal_range(module->GetIdentityHash());
@@ -220,11 +230,14 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
220230

221231
if (synthetic) {
222232
obj->synthetic_ = true;
223-
obj->synthetic_evaluation_steps_.Reset(
224-
env->isolate(), args[3].As<Function>());
233+
obj->object()->SetInternalField(kSyntheticEvaluationStepsSlot, args[3]);
225234
}
226235

227-
obj->context_.Reset(isolate, context);
236+
// Use the extras object as an object whose CreationContext() will be the
237+
// original `context`, since the `Context` itself strictly speaking cannot
238+
// be stored in an internal field.
239+
obj->object()->SetInternalField(kContextObjectSlot,
240+
context->GetExtrasBindingObject());
228241
obj->contextify_context_ = contextify_context;
229242

230243
env->hash_to_module_map.emplace(module->GetIdentityHash(), obj);
@@ -254,7 +267,7 @@ void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
254267

255268
Local<Function> resolver_arg = args[0].As<Function>();
256269

257-
Local<Context> mod_context = obj->context_.Get(isolate);
270+
Local<Context> mod_context = obj->context();
258271
Local<Module> module = obj->module_.Get(isolate);
259272

260273
const int module_requests_length = module->GetModuleRequestsLength();
@@ -295,7 +308,7 @@ void ModuleWrap::Instantiate(const FunctionCallbackInfo<Value>& args) {
295308
Isolate* isolate = args.GetIsolate();
296309
ModuleWrap* obj;
297310
ASSIGN_OR_RETURN_UNWRAP(&obj, args.This());
298-
Local<Context> context = obj->context_.Get(isolate);
311+
Local<Context> context = obj->context();
299312
Local<Module> module = obj->module_.Get(isolate);
300313
TryCatchScope try_catch(env);
301314
USE(module->InstantiateModule(context, ResolveCallback));
@@ -318,7 +331,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
318331
Isolate* isolate = env->isolate();
319332
ModuleWrap* obj;
320333
ASSIGN_OR_RETURN_UNWRAP(&obj, args.This());
321-
Local<Context> context = obj->context_.Get(isolate);
334+
Local<Context> context = obj->context();
322335
Local<Module> module = obj->module_.Get(isolate);
323336

324337
ContextifyContext* contextify_context = obj->contextify_context_;
@@ -636,8 +649,10 @@ MaybeLocal<Value> ModuleWrap::SyntheticModuleEvaluationStepsCallback(
636649

637650
TryCatchScope try_catch(env);
638651
Local<Function> synthetic_evaluation_steps =
639-
obj->synthetic_evaluation_steps_.Get(isolate);
640-
obj->synthetic_evaluation_steps_.Reset();
652+
obj->object()->GetInternalField(kSyntheticEvaluationStepsSlot)
653+
.As<Function>();
654+
obj->object()->SetInternalField(
655+
kSyntheticEvaluationStepsSlot, Undefined(isolate));
641656
MaybeLocal<Value> ret = synthetic_evaluation_steps->Call(context,
642657
obj->object(), 0, nullptr);
643658
if (ret.IsEmpty()) {

src/module_wrap.h

+9-4
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,14 @@ enum HostDefinedOptions : int {
3232

3333
class ModuleWrap : public BaseObject {
3434
public:
35+
enum InternalFields {
36+
kModuleWrapBaseField = BaseObject::kInternalFieldCount,
37+
kURLSlot,
38+
kSyntheticEvaluationStepsSlot,
39+
kContextObjectSlot, // Object whose creation context is the target Context
40+
kInternalFieldCount
41+
};
42+
3543
static void Initialize(v8::Local<v8::Object> target,
3644
v8::Local<v8::Value> unused,
3745
v8::Local<v8::Context> context,
@@ -42,11 +50,11 @@ class ModuleWrap : public BaseObject {
4250
v8::Local<v8::Object> meta);
4351

4452
void MemoryInfo(MemoryTracker* tracker) const override {
45-
tracker->TrackField("url", url_);
4653
tracker->TrackField("resolve_cache", resolve_cache_);
4754
}
4855

4956
inline uint32_t id() { return id_; }
57+
v8::Local<v8::Context> context() const;
5058
static ModuleWrap* GetFromID(node::Environment*, uint32_t id);
5159

5260
SET_MEMORY_INFO_NAME(ModuleWrap)
@@ -85,11 +93,8 @@ class ModuleWrap : public BaseObject {
8593
v8::Local<v8::Module> referrer);
8694
static ModuleWrap* GetFromModule(node::Environment*, v8::Local<v8::Module>);
8795

88-
v8::Global<v8::Function> synthetic_evaluation_steps_;
8996
v8::Global<v8::Module> module_;
90-
v8::Global<v8::String> url_;
9197
std::unordered_map<std::string, v8::Global<v8::Promise>> resolve_cache_;
92-
v8::Global<v8::Context> context_;
9398
contextify::ContextifyContext* contextify_context_ = nullptr;
9499
bool synthetic_ = false;
95100
bool linked_ = false;

0 commit comments

Comments
 (0)