Skip to content

Commit d97041f

Browse files
nigrosimonenodejs-github-bot
nigrosimone
authored and
nodejs-github-bot
committed
src: reduce InternalCallbackScope overhead
InternalCallbackScope looks up the Environment from the isolate two times per call, inside async_context_frame::exchange, and it keeps the prior async context frame in a v8::Global also when there is no frame, that is the common case. Every call from native code into JS pays this: MakeCallback, CallbackScope, AsyncWrap, Node-API. Now the scope passes the Environment it already has, the option is read with an inline accessor instead of copying the shared_ptr, and the global handle is created only when the prior frame is not undefined. benchmark/napi/make_callback, Node 26.3.0 built with and without this change, Linux x64, 30 runs: from 202-208 ns to 155-159 ns per call. Refs: nodejs/performance#24 Signed-off-by: Nigro Simone <nigro.simone@gmail.com> PR-URL: #66316 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 296584b commit d97041f

5 files changed

Lines changed: 28 additions & 10 deletions

File tree

‎src/api/callback.cc‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,8 +112,12 @@ InternalCallbackScope::InternalCallbackScope(
112112

113113
isolate->SetIdle(false);
114114

115-
prior_context_frame_.Reset(
116-
isolate, async_context_frame::exchange(isolate, context_frame));
115+
// The prior frame is usually undefined: no global handle then.
116+
Local<Value> prior_context_frame =
117+
async_context_frame::exchange(env, context_frame);
118+
if (!prior_context_frame->IsUndefined()) {
119+
prior_context_frame_.Reset(isolate, prior_context_frame);
120+
}
117121

118122
env->async_hooks()->push_async_context(
119123
async_context_.async_id, async_context_.trigger_async_id, object);
@@ -159,7 +163,7 @@ void InternalCallbackScope::Close() {
159163
if (pushed_ids_) {
160164
env_->async_hooks()->pop_async_context(async_context_.async_id);
161165

162-
async_context_frame::exchange(isolate, prior_context_frame_.Get(isolate));
166+
async_context_frame::set(env_, prior_context_frame_.Get(isolate));
163167
}
164168

165169
if (failed_) return;

‎src/async_context_frame.cc‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,23 +37,30 @@ Local<Value> current(Isolate* isolate) {
3737
return isolate->GetContinuationPreservedEmbedderDataV2().As<Value>();
3838
}
3939

40-
void set(Isolate* isolate, Local<Value> value) {
41-
auto env = Environment::GetCurrent(isolate);
42-
if (!env->options()->async_context_frame) {
40+
void set(Environment* env, Local<Value> value) {
41+
if (!env->async_context_frame_enabled()) {
4342
return;
4443
}
4544

46-
isolate->SetContinuationPreservedEmbedderDataV2(value);
45+
env->isolate()->SetContinuationPreservedEmbedderDataV2(value);
46+
}
47+
48+
void set(Isolate* isolate, Local<Value> value) {
49+
set(Environment::GetCurrent(isolate), value);
4750
}
4851

4952
// NOTE: It's generally recommended to use async_context_frame::Scope
5053
// but sometimes (such as enterWith) a direct exchange is needed.
51-
Local<Value> exchange(Isolate* isolate, Local<Value> value) {
52-
auto prior = current(isolate);
53-
set(isolate, value);
54+
Local<Value> exchange(Environment* env, Local<Value> value) {
55+
auto prior = current(env->isolate());
56+
set(env, value);
5457
return prior;
5558
}
5659

60+
Local<Value> exchange(Isolate* isolate, Local<Value> value) {
61+
return exchange(Environment::GetCurrent(isolate), value);
62+
}
63+
5764
void CreatePerContextProperties(Local<Object> target,
5865
Local<Value> unused,
5966
Local<Context> context,

‎src/async_context_frame.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ class Scope {
2323

2424
v8::Local<v8::Value> current(v8::Isolate* isolate);
2525
void set(v8::Isolate* isolate, v8::Local<v8::Value> value);
26+
void set(Environment* env, v8::Local<v8::Value> value);
2627
v8::Local<v8::Value> exchange(v8::Isolate* isolate, v8::Local<v8::Value> value);
28+
v8::Local<v8::Value> exchange(Environment* env, v8::Local<v8::Value> value);
2729

2830
} // namespace async_context_frame
2931
} // namespace node

‎src/env-inl.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,10 @@ inline std::shared_ptr<EnvironmentOptions> Environment::options() {
460460
return options_;
461461
}
462462

463+
inline bool Environment::async_context_frame_enabled() const {
464+
return options_->async_context_frame;
465+
}
466+
463467
inline const std::vector<std::string>& Environment::argv() {
464468
return argv_;
465469
}

‎src/env.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1113,6 +1113,7 @@ class Environment final : public MemoryRetainer {
11131113
void* data);
11141114

11151115
inline std::shared_ptr<EnvironmentOptions> options();
1116+
inline bool async_context_frame_enabled() const;
11161117
inline std::shared_ptr<ExclusiveAccess<HostPort>> inspector_host_port();
11171118

11181119
inline int64_t stack_trace_limit() const;

0 commit comments

Comments
 (0)