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

src: track async resources via pointers to stack-allocated handles · nodejs/node@be2a5e1 · GitHub

/ node Public

Commit be2a5e1

Browse files
addaleax
authored and
richardlau
committed
src: track async resources via pointers to stack-allocated handles
This addresses an existing `TODO` to remove the need for a separate `LocalVector`. Since all relevant handles are already present on the stack, we can track their addresses instead of storing the objects in a separate container. PR-URL: #59704 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent 129a1d6 commit be2a5e1

7 files changed

Lines changed: 47 additions & 34 deletions

File tree

‎src/api/callback.cc‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,17 +48,26 @@ InternalCallbackScope::InternalCallbackScope(AsyncWrap* async_wrap, int flags)
4848
flags,
4949
async_wrap->context_frame()) {}
5050

51-
InternalCallbackScope::InternalCallbackScope(Environment* env,
52-
Local<Object> object,
53-
const async_context& asyncContext,
54-
int flags,
55-
Local<Value> context_frame)
51+
InternalCallbackScope::InternalCallbackScope(
52+
Environment* env,
53+
std::variant<Local<Object>, Local<Object>*> object,
54+
const async_context& asyncContext,
55+
int flags,
56+
Local<Value> context_frame)
5657
: env_(env),
5758
async_context_(asyncContext),
58-
object_(object),
5959
skip_hooks_(flags & kSkipAsyncHooks),
6060
skip_task_queues_(flags & kSkipTaskQueues) {
6161
CHECK_NOT_NULL(env);
62+
63+
if (std::holds_alternative<Local<Object>>(object)) {
64+
object_storage_ = std::get<Local<Object>>(object);
65+
object_ = &object_storage_;
66+
} else {
67+
object_ = std::get<Local<Object>*>(object);
68+
CHECK_NOT_NULL(object_);
69+
}
70+
6271
env->PushAsyncCallbackScope();
6372

6473
if (!env->can_call_into_js()) {
@@ -85,7 +94,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
8594
isolate, async_context_frame::exchange(isolate, context_frame));
8695

8796
env->async_hooks()->push_async_context(
88-
async_context_.async_id, async_context_.trigger_async_id, object);
97+
async_context_.async_id, async_context_.trigger_async_id, object_);
8998

9099
pushed_ids_ = true;
91100

‎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, {});
281+
env->async_hooks()->push_async_context(async_id, trigger_async_id, nullptr);
282282
}
283283

284284

‎src/env-inl.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,10 @@ v8::Local<v8::Array> AsyncHooks::js_execution_async_resources() {
141141
}
142142

143143
v8::Local<v8::Object> AsyncHooks::native_execution_async_resource(size_t i) {
144-
if (i >= native_execution_async_resources_.size()) return {};
145-
return native_execution_async_resources_[i];
144+
if (i >= native_execution_async_resources_.size() ||
145+
native_execution_async_resources_[i] == nullptr)
146+
return {};
147+
return *native_execution_async_resources_[i];
146148
}
147149

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

‎src/env.cc‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,8 @@ void Environment::ResetPromiseHooks(Local<Function> init,
120120
// Remember to keep this code aligned with pushAsyncContext() in JS.
121121
void AsyncHooks::push_async_context(double async_id,
122122
double trigger_async_id,
123-
Local<Object> resource) {
123+
Local<Object>* resource) {
124+
CHECK_IMPLIES(resource != nullptr, !resource->IsEmpty());
124125
// Since async_hooks is experimental, do only perform the check
125126
// when async_hooks is enabled.
126127
if (fields_[kCheck] > 0) {
@@ -138,14 +139,14 @@ void AsyncHooks::push_async_context(double async_id,
138139

139140
#ifdef DEBUG
140141
for (uint32_t i = offset; i < native_execution_async_resources_.size(); i++)
141-
CHECK(native_execution_async_resources_[i].IsEmpty());
142+
CHECK_NULL(native_execution_async_resources_[i]);
142143
#endif
143144

144145
// When this call comes from JS (as a way of increasing the stack size),
145146
// `resource` will be empty, because JS caches these values anyway.
146-
if (!resource.IsEmpty()) {
147+
if (resource != nullptr) {
147148
native_execution_async_resources_.resize(offset + 1);
148-
// Caveat: This is a v8::Local<> assignment, we do not keep a v8::Global<>!
149+
// Caveat: This is a v8::Local<>* assignment, we do not keep a v8::Global<>!
149150
native_execution_async_resources_[offset] = resource;
150151
}
151152
}
@@ -170,11 +171,11 @@ bool AsyncHooks::pop_async_context(double async_id) {
170171
fields_[kStackLength] = offset;
171172

172173
if (offset < native_execution_async_resources_.size() &&
173-
!native_execution_async_resources_[offset].IsEmpty()) [[likely]] {
174+
native_execution_async_resources_[offset] != nullptr) [[likely]] {
174175
#ifdef DEBUG
175176
for (uint32_t i = offset + 1; i < native_execution_async_resources_.size();
176177
i++) {
177-
CHECK(native_execution_async_resources_[i].IsEmpty());
178+
CHECK_NULL(native_execution_async_resources_[i]);
178179
}
179180
#endif
180181
native_execution_async_resources_.resize(offset);
@@ -1740,7 +1741,6 @@ AsyncHooks::AsyncHooks(Isolate* isolate, const SerializeInfo* info)
17401741
fields_(isolate, kFieldsCount, MAYBE_FIELD_PTR(info, fields)),
17411742
async_id_fields_(
17421743
isolate, kUidFieldsCount, MAYBE_FIELD_PTR(info, async_id_fields)),
1743-
native_execution_async_resources_(isolate),
17441744
info_(info) {
17451745
HandleScope handle_scope(isolate);
17461746
if (info == nullptr) {
@@ -1829,10 +1829,9 @@ AsyncHooks::SerializeInfo AsyncHooks::Serialize(Local<Context> context,
18291829
native_execution_async_resources_.size());
18301830
for (size_t i = 0; i < native_execution_async_resources_.size(); i++) {
18311831
info.native_execution_async_resources[i] =
1832-
native_execution_async_resources_[i].IsEmpty() ? SIZE_MAX :
1833-
creator->AddData(
1834-
context,
1835-
native_execution_async_resources_[i]);
1832+
native_execution_async_resources_[i] == nullptr
1833+
? SIZE_MAX
1834+
: creator->AddData(context, *native_execution_async_resources_[i]);
18361835
}
18371836

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

‎src/env.h‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
#include <array>
5959
#include <atomic>
6060
#include <cstdint>
61+
#include <deque>
6162
#include <functional>
6263
#include <list>
6364
#include <memory>
@@ -345,7 +346,7 @@ class AsyncHooks : public MemoryRetainer {
345346
// `pop_async_context()` or `clear_async_id_stack()` are called.
346347
void push_async_context(double async_id,
347348
double trigger_async_id,
348-
v8::Local<v8::Object> execution_async_resource);
349+
v8::Local<v8::Object>* execution_async_resource);
349350
bool pop_async_context(double async_id);
350351
void clear_async_id_stack(); // Used in fatal exceptions.
351352

@@ -407,15 +408,9 @@ class AsyncHooks : public MemoryRetainer {
407408

408409
v8::Global<v8::Array> js_execution_async_resources_;
409410

410-
// TODO(@jasnell): Note that this is technically illegal use of
411-
// v8::Locals which should be kept on the stack. Here, the entries
412-
// in this object grows and shrinks with the C stack, and entries
413-
// will be in the right handle scopes, but v8::Locals are supposed
414-
// to remain on the stack and not the heap. For general purposes
415-
// this *should* be ok but may need to be looked at further should
416-
// v8 become stricter in the future about v8::Locals being held in
417-
// the stack.
418-
v8::LocalVector<v8::Object> native_execution_async_resources_;
411+
// We avoid storing the handles directly here, because they are already
412+
// properly allocated on the stack, we just need access to them here.
413+
std::deque<v8::Local<v8::Object>*> native_execution_async_resources_;
419414

420415
// Non-empty during deserialization
421416
const SerializeInfo* info_ = nullptr;

‎src/node_internals.h‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
#include <cstdlib>
3838

3939
#include <string>
40+
#include <variant>
4041
#include <vector>
4142

4243
struct sockaddr;
@@ -245,9 +246,14 @@ class InternalCallbackScope {
245246
// compatibility issues, but it shouldn't.)
246247
kSkipTaskQueues = 2
247248
};
249+
// You need to either guarantee that this `InternalCallbackScope` is
250+
// stack-allocated itself, OR that `object` is a pointer to a stack-allocated
251+
// `v8::Local<v8::Object>` which outlives this scope (e.g. for the
252+
// public `CallbackScope` which indirectly allocates an instance of
253+
// this class for ABI stability purposes).
248254
InternalCallbackScope(
249255
Environment* env,
250-
v8::Local<v8::Object> object,
256+
std::variant<v8::Local<v8::Object>, v8::Local<v8::Object>*> object,
251257
const async_context& asyncContext,
252258
int flags = kNoFlags,
253259
v8::Local<v8::Value> context_frame = v8::Local<v8::Value>());
@@ -263,7 +269,8 @@ class InternalCallbackScope {
263269
private:
264270
Environment* env_;
265271
async_context async_context_;
266-
v8::Local<v8::Object> object_;
272+
v8::Local<v8::Object> object_storage_;
273+
v8::Local<v8::Object>* object_;
267274
bool skip_hooks_;
268275
bool skip_task_queues_;
269276
bool failed_ = false;

‎src/node_task_queue.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,10 +100,11 @@ void PromiseRejectCallback(PromiseRejectMessage message) {
100100
if (!GetAssignedPromiseAsyncId(env, promise, env->trigger_async_id_symbol())
101101
.To(&trigger_async_id)) return;
102102

103+
Local<Object> promise_as_obj = promise;
103104
if (async_id != AsyncWrap::kInvalidAsyncId &&
104105
trigger_async_id != AsyncWrap::kInvalidAsyncId) {
105106
env->async_hooks()->push_async_context(
106-
async_id, trigger_async_id, promise);
107+
async_id, trigger_async_id, &promise_as_obj);
107108
}
108109

109110
USE(callback->Call(

0 commit comments

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