From 728c0def4b52368efd3385517bbc624b1a929666 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 17 Jul 2022 22:13:00 +0530 Subject: [PATCH 1/7] src: use a typed array internally for process._exiting This would prevent manual writes to the _exiting JS property on the process object by passing the data directly via a typed array for performance. This change partially addresses this TODO: https://github.com/nodejs/node/blob/3d575a4f1bd197c3ce669758a2a3c763462a883a/src/api/hooks.cc#L68-L71 Signed-off-by: Darshan Sen --- lib/internal/bootstrap/node.js | 11 +++++++++++ src/api/hooks.cc | 13 +++++-------- src/env-inl.h | 8 ++++++++ src/env.cc | 8 ++++++++ src/env.h | 8 ++++++++ src/node_process_object.cc | 5 +++++ 6 files changed, 45 insertions(+), 8 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index ca5ff569a14117..ff679ea57ba6e6 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -67,6 +67,17 @@ setupGlobalProxy(); setupBuffer(); process.domain = null; +ObjectDefineProperty(process, '_exiting', { + __proto__: null, + get() { + return process._exitingAliasedUint32Array[0] === 1; + }, + set(value) { + process._exitingAliasedUint32Array[0] = value ? 1 : 0; + }, + enumerable: true, + configurable: true, +}); process._exiting = false; // process.config is serialized config.gypi diff --git a/src/api/hooks.cc b/src/api/hooks.cc index bd26e6d150d53e..9e54436ba30fb6 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -65,14 +65,11 @@ Maybe EmitProcessExit(Environment* env) { Context::Scope context_scope(context); Local process_object = env->process_object(); - // TODO(addaleax): It might be nice to share process._exiting and - // process.exitCode via getter/setter pairs that pass data directly to the - // native side, so that we don't manually have to read and write JS properties - // here. These getters could use e.g. a typed array for performance. - if (process_object - ->Set(context, - FIXED_ONE_BYTE_STRING(isolate, "_exiting"), - True(isolate)).IsNothing()) return Nothing(); + // TODO(addaleax): It might be nice to share process.exitCode via + // getter/setter pairs that pass data directly to the native side, so that we + // don't manually have to read and write JS properties here. These getters + // could use e.g. a typed array for performance. + env->set_exiting(true); Local exit_code = env->exit_code_string(); Local code_v; diff --git a/src/env-inl.h b/src/env-inl.h index 084cb9f83c51af..c49ca74ec060ce 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -371,6 +371,14 @@ inline bool Environment::force_context_aware() const { return options_->force_context_aware; } +inline void Environment::set_exiting(bool value) { + exiting_[0] = value ? 1 : 0; +} + +inline AliasedUint32Array& Environment::exiting() { + return exiting_; +} + inline void Environment::set_abort_on_uncaught_exception(bool value) { options_->abort_on_uncaught_exception = value; } diff --git a/src/env.cc b/src/env.cc index e4a708ec1de645..e435a8ff5f8b3e 100644 --- a/src/env.cc +++ b/src/env.cc @@ -736,6 +736,7 @@ Environment::Environment(IsolateData* isolate_data, exec_argv_(exec_args), argv_(args), exec_path_(GetExecPath(args)), + exiting_(isolate_, 1, MAYBE_FIELD_PTR(env_info, exiting)), should_abort_on_uncaught_toggle_( isolate_, 1, @@ -843,6 +844,9 @@ void Environment::InitializeMainContext(Local context, // By default, always abort when --abort-on-uncaught-exception was passed. should_abort_on_uncaught_toggle_[0] = 1; + // The process is not exiting by default. + set_exiting(false); + performance_state_->Mark(performance::NODE_PERFORMANCE_MILESTONE_ENVIRONMENT, time_origin_); performance_state_->Mark(performance::NODE_PERFORMANCE_MILESTONE_NODE_START, @@ -1744,6 +1748,7 @@ EnvSerializeInfo Environment::Serialize(SnapshotCreator* creator) { info.immediate_info = immediate_info_.Serialize(ctx, creator); info.tick_info = tick_info_.Serialize(ctx, creator); info.performance_state = performance_state_->Serialize(ctx, creator); + info.exiting = exiting_.Serialize(ctx, creator); info.stream_base_state = stream_base_state_.Serialize(ctx, creator); info.should_abort_on_uncaught_toggle = should_abort_on_uncaught_toggle_.Serialize(ctx, creator); @@ -1815,6 +1820,7 @@ std::ostream& operator<<(std::ostream& output, const EnvSerializeInfo& i) { << "// -- performance_state begins --\n" << i.performance_state << ",\n" << "// -- performance_state ends --\n" + << i.exiting << ", // exiting\n" << i.stream_base_state << ", // stream_base_state\n" << i.should_abort_on_uncaught_toggle << ", // should_abort_on_uncaught_toggle\n" @@ -1861,6 +1867,7 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info) { immediate_info_.Deserialize(ctx); tick_info_.Deserialize(ctx); performance_state_->Deserialize(ctx); + exiting_.Deserialize(ctx); stream_base_state_.Deserialize(ctx); should_abort_on_uncaught_toggle_.Deserialize(ctx); @@ -2091,6 +2098,7 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { native_modules_without_cache); tracker->TrackField("destroy_async_id_list", destroy_async_id_list_); tracker->TrackField("exec_argv", exec_argv_); + tracker->TrackField("exiting", exiting_); tracker->TrackField("should_abort_on_uncaught_toggle", should_abort_on_uncaught_toggle_); tracker->TrackField("stream_base_state", stream_base_state_); diff --git a/src/env.h b/src/env.h index 04528583cb47f4..3fb1254214da80 100644 --- a/src/env.h +++ b/src/env.h @@ -952,6 +952,7 @@ struct EnvSerializeInfo { TickInfo::SerializeInfo tick_info; ImmediateInfo::SerializeInfo immediate_info; performance::PerformanceState::SerializeInfo performance_state; + AliasedBufferIndex exiting; AliasedBufferIndex stream_base_state; AliasedBufferIndex should_abort_on_uncaught_toggle; @@ -1153,6 +1154,11 @@ class Environment : public MemoryRetainer { inline void set_force_context_aware(bool value); inline bool force_context_aware() const; + // This is a pseudo-boolean that keeps track of whether the process is + // exiting. + inline void set_exiting(bool value); + inline AliasedUint32Array& exiting(); + // This stores whether the --abort-on-uncaught-exception flag was passed // to Node. inline bool abort_on_uncaught_exception() const; @@ -1550,6 +1556,8 @@ class Environment : public MemoryRetainer { uint32_t script_id_counter_ = 0; uint32_t function_id_counter_ = 0; + AliasedUint32Array exiting_; + AliasedUint32Array should_abort_on_uncaught_toggle_; int should_not_abort_scope_counter_ = 0; diff --git a/src/node_process_object.cc b/src/node_process_object.cc index 29f6569a45e5b2..9c0f8879bcccce 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -91,6 +91,11 @@ MaybeLocal CreateProcessObject(Environment* env) { return MaybeLocal(); } + // process._exitingAliasedUint32Array + Local exiting_string = + FIXED_ONE_BYTE_STRING(env->isolate(), "_exitingAliasedUint32Array"); + process->Set(context, exiting_string, env->exiting().GetJSArray()).FromJust(); + // process.version READONLY_PROPERTY(process, "version", From 99f64038c2a3a5f2db0c314e4ebbf1c8f6b47906 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Mon, 18 Jul 2022 18:34:57 +0530 Subject: [PATCH 2/7] Update src/node_process_object.cc Co-authored-by: Anna Henningsen --- src/node_process_object.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node_process_object.cc b/src/node_process_object.cc index 9c0f8879bcccce..258bf08b6b48fd 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -94,7 +94,9 @@ MaybeLocal CreateProcessObject(Environment* env) { // process._exitingAliasedUint32Array Local exiting_string = FIXED_ONE_BYTE_STRING(env->isolate(), "_exitingAliasedUint32Array"); - process->Set(context, exiting_string, env->exiting().GetJSArray()).FromJust(); + if (process->Set(context, exiting_string, env->exiting().GetJSArray()).IsNothing()) { + return {}; + } // process.version READONLY_PROPERTY(process, From 20ad6c1152284dded8db08bc09c74a42f86aec1c Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Mon, 18 Jul 2022 18:36:19 +0530 Subject: [PATCH 3/7] fixup! Update src/node_process_object.cc Signed-off-by: Darshan Sen --- src/node_process_object.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_process_object.cc b/src/node_process_object.cc index 258bf08b6b48fd..5fbfb4acea9252 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -94,7 +94,8 @@ MaybeLocal CreateProcessObject(Environment* env) { // process._exitingAliasedUint32Array Local exiting_string = FIXED_ONE_BYTE_STRING(env->isolate(), "_exitingAliasedUint32Array"); - if (process->Set(context, exiting_string, env->exiting().GetJSArray()).IsNothing()) { + if (process->Set(context, exiting_string, env->exiting().GetJSArray()) + .IsNothing()) { return {}; } From 7d3b32ea375f5b52543ef352d5df0c05f85f615e Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Fri, 22 Jul 2022 19:08:36 +0530 Subject: [PATCH 4/7] src: use a private symbol instead of a string key Signed-off-by: Darshan Sen --- lib/internal/bootstrap/node.js | 8 ++++++-- src/env.h | 1 + src/node_process_object.cc | 9 +++++---- typings/internalBinding/util.d.ts | 1 + 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index ff679ea57ba6e6..6dabdc19344d89 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -60,6 +60,10 @@ const { deprecate, exposeInterface, } = require('internal/util'); +const { + exiting_aliased_Uint32Array, + getHiddenValue, +} = internalBinding('util'); setupProcessObject(); @@ -70,10 +74,10 @@ process.domain = null; ObjectDefineProperty(process, '_exiting', { __proto__: null, get() { - return process._exitingAliasedUint32Array[0] === 1; + return getHiddenValue(process, exiting_aliased_Uint32Array)[0] === 1; }, set(value) { - process._exitingAliasedUint32Array[0] = value ? 1 : 0; + getHiddenValue(process, exiting_aliased_Uint32Array)[0] = value ? 1 : 0; }, enumerable: true, configurable: true, diff --git a/src/env.h b/src/env.h index 3fb1254214da80..14caf2c626809f 100644 --- a/src/env.h +++ b/src/env.h @@ -173,6 +173,7 @@ class NoArrayBufferZeroFillScope { V(napi_type_tag, "node:napi:type_tag") \ V(napi_wrapper, "node:napi:wrapper") \ V(untransferable_object_private_symbol, "node:untransferableObject") \ + V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array") // Symbols are per-isolate primitives but Environment proxies them // for the sake of convenience. diff --git a/src/node_process_object.cc b/src/node_process_object.cc index 5fbfb4acea9252..3a44c45c6b83e2 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -91,10 +91,11 @@ MaybeLocal CreateProcessObject(Environment* env) { return MaybeLocal(); } - // process._exitingAliasedUint32Array - Local exiting_string = - FIXED_ONE_BYTE_STRING(env->isolate(), "_exitingAliasedUint32Array"); - if (process->Set(context, exiting_string, env->exiting().GetJSArray()) + // process[exiting_aliased_Uint32Array] + if (process + ->SetPrivate(context, + env->exiting_aliased_Uint32Array(), + env->exiting().GetJSArray()) .IsNothing()) { return {}; } diff --git a/typings/internalBinding/util.d.ts b/typings/internalBinding/util.d.ts index 40def32d570d55..bb85acee21a458 100644 --- a/typings/internalBinding/util.d.ts +++ b/typings/internalBinding/util.d.ts @@ -17,6 +17,7 @@ declare function InternalBinding(binding: 'util'): { napi_type_tag: 5; napi_wrapper: 6; untransferable_object_private_symbol: 7; + exiting_aliased_Uint32Array: 8; kPending: 0; kFulfilled: 1; From 105877614e8c692e08ae0e3051884c5c8191de03 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Fri, 22 Jul 2022 19:12:36 +0530 Subject: [PATCH 5/7] fixup! clang-format fix Signed-off-by: Darshan Sen --- src/env.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/env.h b/src/env.h index 14caf2c626809f..a96e542948e723 100644 --- a/src/env.h +++ b/src/env.h @@ -164,15 +164,15 @@ class NoArrayBufferZeroFillScope { // Private symbols are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only and have a // "node:" prefix to avoid name clashes with third-party code. -#define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) \ - V(alpn_buffer_private_symbol, "node:alpnBuffer") \ - V(arrow_message_private_symbol, "node:arrowMessage") \ - V(contextify_context_private_symbol, "node:contextify:context") \ - V(contextify_global_private_symbol, "node:contextify:global") \ - V(decorated_private_symbol, "node:decorated") \ - V(napi_type_tag, "node:napi:type_tag") \ - V(napi_wrapper, "node:napi:wrapper") \ - V(untransferable_object_private_symbol, "node:untransferableObject") \ +#define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) \ + V(alpn_buffer_private_symbol, "node:alpnBuffer") \ + V(arrow_message_private_symbol, "node:arrowMessage") \ + V(contextify_context_private_symbol, "node:contextify:context") \ + V(contextify_global_private_symbol, "node:contextify:global") \ + V(decorated_private_symbol, "node:decorated") \ + V(napi_type_tag, "node:napi:type_tag") \ + V(napi_wrapper, "node:napi:wrapper") \ + V(untransferable_object_private_symbol, "node:untransferableObject") \ V(exiting_aliased_Uint32Array, "node:exiting_aliased_Uint32Array") // Symbols are per-isolate primitives but Environment proxies them From 18b29f2f8f0e9f59f9e20cfebc636306151a3122 Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 24 Jul 2022 13:34:01 +0530 Subject: [PATCH 6/7] fixup! stop calling getHiddenValue() every time process._exiting is accessed Signed-off-by: Darshan Sen --- lib/internal/bootstrap/node.js | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 6dabdc19344d89..3401e5dac99572 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -71,17 +71,21 @@ setupGlobalProxy(); setupBuffer(); process.domain = null; -ObjectDefineProperty(process, '_exiting', { - __proto__: null, - get() { - return getHiddenValue(process, exiting_aliased_Uint32Array)[0] === 1; - }, - set(value) { - getHiddenValue(process, exiting_aliased_Uint32Array)[0] = value ? 1 : 0; - }, - enumerable: true, - configurable: true, -}); +{ + const existingAliasedUint32Array = + getHiddenValue(process, exiting_aliased_Uint32Array); + ObjectDefineProperty(process, '_exiting', { + __proto__: null, + get() { + return existingAliasedUint32Array[0] === 1; + }, + set(value) { + existingAliasedUint32Array[0] = value ? 1 : 0; + }, + enumerable: true, + configurable: true, + }); +} process._exiting = false; // process.config is serialized config.gypi From a9e68be3f90a0a35eeb49a21c0e02b087ea2180e Mon Sep 17 00:00:00 2001 From: Darshan Sen Date: Sun, 24 Jul 2022 13:38:23 +0530 Subject: [PATCH 7/7] fixup! fixup! stop calling getHiddenValue() every time process._exiting is accessed Signed-off-by: Darshan Sen --- lib/internal/bootstrap/node.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 3401e5dac99572..df2afb84b5a6ca 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -72,15 +72,15 @@ setupBuffer(); process.domain = null; { - const existingAliasedUint32Array = + const exitingAliasedUint32Array = getHiddenValue(process, exiting_aliased_Uint32Array); ObjectDefineProperty(process, '_exiting', { __proto__: null, get() { - return existingAliasedUint32Array[0] === 1; + return exitingAliasedUint32Array[0] === 1; }, set(value) { - existingAliasedUint32Array[0] = value ? 1 : 0; + exitingAliasedUint32Array[0] = value ? 1 : 0; }, enumerable: true, configurable: true,