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

src: use `Global` for storing resource in Node-API callback scope · nodejs/node@ce748f6 · GitHub

/ node Public

Commit ce748f6

Browse files
addaleax
authored and
nodejs-github-bot
committed
src: use Global for storing resource in Node-API callback scope
This is a follow-up to 234c26c. The Node-API interface does not allow us to enforce that values are stored in a specific location, e.g. on the stack or not; however, V8 requires `Local<>` handles to be stored on the stack. To circumvent this restriction, we add the ability to the async handle stack to store either `Local<>*` pointers or `Global<>*` pointers, with Node-API making use of the latter. PR-URL: #59828 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent 3e79dba commit ce748f6

8 files changed

Lines changed: 110 additions & 46 deletions

File tree

‎src/api/callback.cc‎

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ namespace node {
88

99
using v8::Context;
1010
using v8::Function;
11+
using v8::Global;
1112
using v8::HandleScope;
1213
using v8::Isolate;
1314
using v8::Local;
@@ -26,13 +27,30 @@ CallbackScope::CallbackScope(Isolate* isolate,
2627
CallbackScope::CallbackScope(Environment* env,
2728
Local<Object> object,
2829
async_context asyncContext)
29-
: resource_storage_(object),
30-
private_(
31-
new InternalCallbackScope(env, &resource_storage_, asyncContext)),
30+
: resource_storage_({.local = object}),
31+
private_(new InternalCallbackScope(
32+
env, &resource_storage_.local, asyncContext)),
3233
try_catch_(env->isolate()) {
3334
try_catch_.SetVerbose(true);
3435
}
3536

37+
CallbackScope::CallbackScope(Environment* env,
38+
Global<Object>* object,
39+
async_context asyncContext)
40+
: resource_storage_({.global_ptr = object}),
41+
private_(new InternalCallbackScope(
42+
env, resource_storage_.global_ptr, asyncContext)),
43+
try_catch_(env->isolate()) {
44+
try_catch_.SetVerbose(true);
45+
// These checks can be removed in a future major version -- they ensure
46+
// ABI compatibility with previous Node.js versions.
47+
static_assert(sizeof(resource_storage_) == sizeof(Global<Object>*));
48+
static_assert(sizeof(resource_storage_) == sizeof(Local<Object>));
49+
static_assert(alignof(decltype(resource_storage_)) ==
50+
alignof(Global<Object>*));
51+
static_assert(alignof(decltype(resource_storage_)) == alignof(Local<Object>));
52+
}
53+
3654
CallbackScope::~CallbackScope() {
3755
if (try_catch_.HasCaught())
3856
private_->MarkAsFailed();
@@ -49,7 +67,7 @@ InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap, int flags)
4967

5068
InternalCallbackScope::InternalCallbackScope(
5169
Environment* env,
52-
std::variant<Local<Object>, Local<Object>*> object,
70+
std::variant<Local<Object>, Local<Object>*, Global<Object>*> object_arg,
5371
const async_context& asyncContext,
5472
int flags,
5573
Local<Value> context_frame)
@@ -59,13 +77,16 @@ InternalCallbackScope::InternalCallbackScope(
5977
skip_task_queues_(flags & kSkipTaskQueues) {
6078
CHECK_NOT_NULL(env);
6179

62-
if (std::holds_alternative<Local<Object>>(object)) {
63-
object_storage_ = std::get<Local<Object>>(object);
64-
object_ = &object_storage_;
80+
std::variant<v8::Local<v8::Object>*, v8::Global<v8::Object>*> object;
81+
if (std::holds_alternative<Local<Object>>(object_arg)) {
82+
object_storage_ = std::get<Local<Object>>(object_arg);
83+
object = &object_storage_;
84+
} else if (std::holds_alternative<Local<Object>*>(object_arg)) {
85+
object = std::get<Local<Object>*>(object_arg);
6586
} else {
66-
object_ = std::get<Local<Object>*>(object);
67-
CHECK_NOT_NULL(object_);
87+
object = std::get<Global<Object>*>(object_arg);
6888
}
89+
std::visit([](auto* ptr) { CHECK_NOT_NULL(ptr); }, object);
6990

7091
env->PushAsyncCallbackScope();
7192

@@ -93,7 +114,7 @@ InternalCallbackScope::InternalCallbackScope(
93114
isolate, async_context_frame::exchange(isolate, context_frame));
94115

95116
env->async_hooks()->push_async_context(
96-
async_context_.async_id, async_context_.trigger_async_id, object_);
117+
async_context_.async_id, async_context_.trigger_async_id, object);
97118

98119
pushed_ids_ = true;
99120

‎src/async_wrap.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,7 @@ void AsyncWrap::PushAsyncContext(const FunctionCallbackInfo<Value>& args) {
278278
// then the checks in push_async_ids() and pop_async_id() will.
279279
double async_id = args[0]->NumberValue(env->context()).FromJust();
280280
double trigger_async_id = args[1]->NumberValue(env->context()).FromJust();
281-
env->async_hooks()->push_async_context(async_id, trigger_async_id, nullptr);
281+
env->async_hooks()->push_async_context(async_id, trigger_async_id, {});
282282
}
283283

284284

‎src/env-inl.h‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -106,10 +106,19 @@ v8::Local<v8::Array> AsyncHooks::js_execution_async_resources() {
106106
}
107107

108108
v8::Local<v8::Object> AsyncHooks::native_execution_async_resource(size_t i) {
109-
if (i >= native_execution_async_resources_.size() ||
110-
native_execution_async_resources_[i] == nullptr)
111-
return {};
112-
return *native_execution_async_resources_[i];
109+
if (i >= native_execution_async_resources_.size()) return {};
110+
auto resource = native_execution_async_resources_[i];
111+
if (std::holds_alternative<v8::Global<v8::Object>*>(resource)) [[unlikely]] {
112+
auto* global = std::get<v8::Global<v8::Object>*>(resource);
113+
if (global == nullptr) [[unlikely]]
114+
return {};
115+
return global->Get(env()->isolate());
116+
} else {
117+
auto* local = std::get<v8::Local<v8::Object>*>(resource);
118+
if (local == nullptr) [[unlikely]]
119+
return {};
120+
return *local;
121+
}
113122
}
114123

115124
inline v8::Local<v8::String> AsyncHooks::provider_string(int idx) {

‎src/env.cc‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ using v8::EmbedderGraph;
4848
using v8::EscapableHandleScope;
4949
using v8::ExternalMemoryAccounter;
5050
using v8::Function;
51+
using v8::Global;
5152
using v8::HandleScope;
5253
using v8::HeapProfiler;
5354
using v8::HeapSpaceStatistics;
@@ -120,10 +121,12 @@ void Environment::ResetPromiseHooks(Local<Function> init,
120121
}
121122

122123
// Remember to keep this code aligned with pushAsyncContext() in JS.
123-
void AsyncHooks::push_async_context(double async_id,
124-
double trigger_async_id,
125-
Local<Object>* resource) {
126-
CHECK_IMPLIES(resource != nullptr, !resource->IsEmpty());
124+
void AsyncHooks::push_async_context(
125+
double async_id,
126+
double trigger_async_id,
127+
std::variant<Local<Object>*, Global<Object>*> resource) {
128+
std::visit([](auto* ptr) { CHECK_IMPLIES(ptr != nullptr, !ptr->IsEmpty()); },
129+
resource);
127130
// Since async_hooks is experimental, do only perform the check
128131
// when async_hooks is enabled.
129132
if (fields_[kCheck] > 0) {
@@ -141,12 +144,13 @@ void AsyncHooks::push_async_context(double async_id,
141144

142145
#ifdef DEBUG
143146
for (uint32_t i = offset; i < native_execution_async_resources_.size(); i++)
144-
CHECK_NULL(native_execution_async_resources_[i]);
147+
std::visit([](auto* ptr) { CHECK_NULL(ptr); },
148+
native_execution_async_resources_[i]);
145149
#endif
146150

147151
// When this call comes from JS (as a way of increasing the stack size),
148152
// `resource` will be empty, because JS caches these values anyway.
149-
if (resource != nullptr) {
153+
if (std::visit([](auto* ptr) { return ptr != nullptr; }, resource)) {
150154
native_execution_async_resources_.resize(offset + 1);
151155
// Caveat: This is a v8::Local<>* assignment, we do not keep a v8::Global<>!
152156
native_execution_async_resources_[offset] = resource;
@@ -173,11 +177,13 @@ bool AsyncHooks::pop_async_context(double async_id) {
173177
fields_[kStackLength] = offset;
174178

175179
if (offset < native_execution_async_resources_.size() &&
176-
native_execution_async_resources_[offset] != nullptr) [[likely]] {
180+
std::visit([](auto* ptr) { return ptr != nullptr; },
181+
native_execution_async_resources_[offset])) [[likely]] {
177182
#ifdef DEBUG
178183
for (uint32_t i = offset + 1; i < native_execution_async_resources_.size();
179184
i++) {
180-
CHECK_NULL(native_execution_async_resources_[i]);
185+
std::visit([](auto* ptr) { CHECK_NULL(ptr); },
186+
native_execution_async_resources_[i]);
181187
}
182188
#endif
183189
native_execution_async_resources_.resize(offset);
@@ -1821,10 +1827,9 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> context,
18211827
info.native_execution_async_resources.resize(
18221828
native_execution_async_resources_.size());
18231829
for (size_t i = 0; i < native_execution_async_resources_.size(); i++) {
1830+
auto resource = native_execution_async_resource(i);
18241831
info.native_execution_async_resources[i] =
1825-
native_execution_async_resources_[i] == nullptr
1826-
? SIZE_MAX
1827-
: creator->AddData(context, *native_execution_async_resources_[i]);
1832+
resource.IsEmpty() ? SIZE_MAX : creator->AddData(context, resource);
18281833
}
18291834

18301835
// At the moment, promise hooks are not supported in the startup snapshot.

‎src/env.h‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@
6868
#include <string>
6969
#include <unordered_map>
7070
#include <unordered_set>
71+
#include <variant>
7172
#include <vector>
7273

7374
namespace node {
@@ -323,9 +324,11 @@ class AsyncHooks : public MemoryRetainer {
323324
// NB: This call does not take (co-)ownership of `execution_async_resource`.
324325
// The lifetime of the `v8::Local<>` pointee must last until
325326
// `pop_async_context()` or `clear_async_id_stack()` are called.
326-
void push_async_context(double async_id,
327-
double trigger_async_id,
328-
v8::Local<v8::Object>* execution_async_resource);
327+
void push_async_context(
328+
double async_id,
329+
double trigger_async_id,
330+
std::variant<v8::Local<v8::Object>*, v8::Global<v8::Object>*>
331+
execution_async_resource);
329332
bool pop_async_context(double async_id);
330333
void clear_async_id_stack(); // Used in fatal exceptions.
331334

@@ -389,7 +392,12 @@ class AsyncHooks : public MemoryRetainer {
389392

390393
// We avoid storing the handles directly here, because they are already
391394
// properly allocated on the stack, we just need access to them here.
392-
std::deque<v8::Local<v8::Object>*> native_execution_async_resources_;
395+
// The `v8::Global<>` variant is here because the Node-API API design
396+
// does not allow us to make sure that we exclusively store this value
397+
// on the stack, so we accept the small perf hit that comes with
398+
// global handles in that case.
399+
std::deque<std::variant<v8::Local<v8::Object>*, v8::Global<v8::Object>*>>
400+
native_execution_async_resources_;
393401

394402
// Non-empty during deserialization
395403
const SerializeInfo* info_ = nullptr;

‎src/node.h‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1444,6 +1444,12 @@ class NODE_EXTERN CallbackScope {
14441444
CallbackScope(Environment* env,
14451445
v8::Local<v8::Object> resource,
14461446
async_context asyncContext);
1447+
// `resource` needs to outlive the scope in this case.
1448+
// This is for the rare situation in which `CallbackScope` cannot be
1449+
// stack-allocated. `resource` needs to outlive this scope.
1450+
CallbackScope(Environment* env,
1451+
v8::Global<v8::Object>* resource,
1452+
async_context asyncContext);
14471453
~CallbackScope();
14481454

14491455
void operator=(const CallbackScope&) = delete;
@@ -1452,8 +1458,11 @@ class NODE_EXTERN CallbackScope {
14521458
CallbackScope(CallbackScope&&) = delete;
14531459

14541460
private:
1455-
void* reserved_;
1456-
v8::Local<v8::Object> resource_storage_;
1461+
void* resource_storage_global_;
1462+
union {
1463+
v8::Local<v8::Object> local;
1464+
v8::Global<v8::Object>* global_ptr;
1465+
} resource_storage_;
14571466
InternalCallbackScope* private_;
14581467
v8::TryCatch try_catch_;
14591468
};

‎src/node_api.cc‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -584,10 +584,9 @@ class AsyncContext {
584584

585585
inline napi_callback_scope OpenCallbackScope() {
586586
EnsureReference();
587-
napi_callback_scope it =
588-
reinterpret_cast<napi_callback_scope>(new CallbackScope(this));
587+
auto scope = new HeapAllocatedCallbackScope(this);
589588
env_->open_callback_scopes++;
590-
return it;
589+
return scope->to_opaque();
591590
}
592591

593592
inline void EnsureReference() {
@@ -609,8 +608,7 @@ class AsyncContext {
609608

610609
static inline void CloseCallbackScope(node_napi_env env,
611610
napi_callback_scope s) {
612-
CallbackScope* callback_scope = reinterpret_cast<CallbackScope*>(s);
613-
delete callback_scope;
611+
delete HeapAllocatedCallbackScope::FromOpaque(s);
614612
env->open_callback_scopes--;
615613
}
616614

@@ -621,13 +619,26 @@ class AsyncContext {
621619
}
622620

623621
private:
624-
class CallbackScope : public node::CallbackScope {
622+
class HeapAllocatedCallbackScope final {
625623
public:
626-
explicit CallbackScope(AsyncContext* async_context)
627-
: node::CallbackScope(async_context->node_env(),
628-
async_context->resource_.Get(
629-
async_context->node_env()->isolate()),
630-
async_context->async_context()) {}
624+
napi_callback_scope to_opaque() {
625+
return reinterpret_cast<napi_callback_scope>(this);
626+
}
627+
static HeapAllocatedCallbackScope* FromOpaque(napi_callback_scope s) {
628+
return reinterpret_cast<HeapAllocatedCallbackScope*>(s);
629+
}
630+
631+
explicit HeapAllocatedCallbackScope(AsyncContext* async_context)
632+
: resource_storage_(async_context->node_env()->isolate(),
633+
async_context->resource_.Get(
634+
async_context->node_env()->isolate())),
635+
cs_(async_context->node_env(),
636+
&resource_storage_,
637+
async_context->async_context()) {}
638+
639+
private:
640+
v8::Global<v8::Object> resource_storage_;
641+
node::CallbackScope cs_;
631642
};
632643

633644
node_napi_env env_;

‎src/node_internals.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -250,10 +250,12 @@ class InternalCallbackScope {
250250
// stack-allocated itself, OR that `object` is a pointer to a stack-allocated
251251
// `v8::Local<v8::Object>` which outlives this scope (e.g. for the
252252
// public `CallbackScope` which indirectly allocates an instance of
253-
// this class for ABI stability purposes).
253+
// this class for ABI stability purposes), OR pass a `Global<>`.
254254
InternalCallbackScope(
255255
Environment* env,
256-
std::variant<v8::Local<v8::Object>, v8::Local<v8::Object>*> object,
256+
std::variant<v8::Local<v8::Object>,
257+
v8::Local<v8::Object>*,
258+
v8::Global<v8::Object>*> object,
257259
const async_context& asyncContext,
258260
int flags = kNoFlags,
259261
v8::Local<v8::Value> context_frame = v8::Local<v8::Value>());
@@ -270,7 +272,6 @@ class InternalCallbackScope {
270272
Environment* env_;
271273
async_context async_context_;
272274
v8::Local<v8::Object> object_storage_;
273-
v8::Local<v8::Object>* object_;
274275
bool skip_hooks_;
275276
bool skip_task_queues_;
276277
bool failed_ = false;

0 commit comments

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