Skip to content

Commit daecfc0

Browse files
anonriganonrig
authored
child_process: pass spawn options to the binding positionally
ChildProcess.prototype.spawn() handed a single options object to the ProcessWrap::Spawn() binding, which then read about a dozen properties back out individually with Object::Get(). Each of those is a property lookup across the JS/C++ boundary on every spawn. Pass file, args, cwd, envPairs, stdio, uid and gid as positional arguments and pack the boolean flags (detached, windowsHide, windowsVerbatimArguments) into a single integer whose bit values are exported from the binding as `constants`. The native side then reads each value directly from the call arguments. Add internal typings for the process_wrap binding (previously untyped) describing the new positional spawn() signature and the exported constants. There is no observable behavior change. Spawn wall-clock time is dominated by the operating system process-creation cost and is unchanged; this reduces the per-spawn work done on the main thread and clarifies the contract between the JS and C++ layers. Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com> PR-URL: #63930 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me>
1 parent 5e0969a commit daecfc0

4 files changed

Lines changed: 102 additions & 76 deletions

File tree

‎lib/internal/child_process.js‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ const dgram = require('dgram');
4141
const inspect = require('internal/util/inspect').inspect;
4242
const assert = require('internal/assert');
4343

44-
const { Process } = internalBinding('process_wrap');
44+
const { Process, constants: processConstants } = internalBinding('process_wrap');
4545
const {
4646
WriteWrap,
4747
kReadBytesOrError,
@@ -397,7 +397,24 @@ ChildProcess.prototype.spawn = function spawn(options) {
397397
childProcessSpawn.start.publish({ process: this, options });
398398
}
399399

400-
const err = this._handle.spawn(options);
400+
let spawnFlags = 0;
401+
if (options.detached)
402+
spawnFlags |= processConstants.kProcessFlagDetached;
403+
if (options.windowsHide)
404+
spawnFlags |= processConstants.kProcessFlagWindowsHide;
405+
if (options.windowsVerbatimArguments)
406+
spawnFlags |= processConstants.kProcessFlagWindowsVerbatimArguments;
407+
408+
const err = this._handle.spawn(
409+
options.file,
410+
options.args,
411+
options.cwd,
412+
options.envPairs,
413+
options.stdio,
414+
spawnFlags,
415+
options.uid,
416+
options.gid,
417+
);
401418

402419
// Run-time errors should emit an error, not throw an exception.
403420
if (err === UV_EACCES ||

‎src/process_wrap.cc‎

Lines changed: 38 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,18 @@ using v8::Nothing;
4949
using v8::Number;
5050
using v8::Object;
5151
using v8::String;
52+
using v8::Uint32;
5253
using v8::Value;
5354

5455
namespace {
5556

57+
enum ProcessFlags : uint32_t {
58+
kProcessFlagNone = 0,
59+
kProcessFlagDetached = 1 << 0,
60+
kProcessFlagWindowsHide = 1 << 1,
61+
kProcessFlagWindowsVerbatimArguments = 1 << 2,
62+
};
63+
5664
class ProcessWrap : public HandleWrap {
5765
public:
5866
static void Initialize(Local<Object> target,
@@ -71,6 +79,12 @@ class ProcessWrap : public HandleWrap {
7179
SetProtoMethod(isolate, constructor, "kill", Kill);
7280

7381
SetConstructorFunction(context, target, "Process", constructor);
82+
83+
Local<Object> constants = Object::New(isolate);
84+
NODE_DEFINE_CONSTANT(constants, kProcessFlagDetached);
85+
NODE_DEFINE_CONSTANT(constants, kProcessFlagWindowsHide);
86+
NODE_DEFINE_CONSTANT(constants, kProcessFlagWindowsVerbatimArguments);
87+
target->Set(context, env->constants_string(), constants).Check();
7488
}
7589

7690
static void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
@@ -118,14 +132,9 @@ class ProcessWrap : public HandleWrap {
118132

119133
static Maybe<void> ParseStdioOptions(
120134
Environment* env,
121-
Local<Object> js_options,
135+
Local<Value> stdios_val,
122136
std::vector<uv_stdio_container_t>* options_stdio) {
123137
Local<Context> context = env->context();
124-
Local<String> stdio_key = env->stdio_string();
125-
Local<Value> stdios_val;
126-
if (!js_options->Get(context, stdio_key).ToLocal(&stdios_val)) {
127-
return Nothing<void>();
128-
}
129138
if (!stdios_val->IsArray()) {
130139
THROW_ERR_INVALID_ARG_TYPE(env, "options.stdio must be an array");
131140
return Nothing<void>();
@@ -188,46 +197,30 @@ class ProcessWrap : public HandleWrap {
188197
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.This());
189198
int err = 0;
190199

191-
if (!args[0]->IsObject()) {
192-
return THROW_ERR_INVALID_ARG_TYPE(env, "options must be an object");
193-
}
194-
195-
Local<Object> js_options = args[0].As<Object>();
196-
197200
uv_process_options_t options;
198201
memset(&options, 0, sizeof(uv_process_options_t));
199202

200203
options.exit_cb = OnExit;
201204

202-
// options.file
203-
Local<Value> file_v;
204-
if (!js_options->Get(context, env->file_string()).ToLocal(&file_v)) {
205-
return;
206-
}
207-
CHECK(file_v->IsString());
208-
node::Utf8Value file(env->isolate(), file_v);
205+
// args[0] file
206+
CHECK(args[0]->IsString());
207+
node::Utf8Value file(env->isolate(), args[0]);
209208
options.file = *file;
210209

211210
THROW_IF_INSUFFICIENT_PERMISSIONS(
212211
env, permission::PermissionScope::kChildProcess, file.ToStringView());
213212

214-
// options.uid
215-
Local<Value> uid_v;
216-
if (!js_options->Get(context, env->uid_string()).ToLocal(&uid_v)) {
217-
return;
218-
}
213+
// args[6] uid
214+
Local<Value> uid_v = args[6];
219215
if (!uid_v->IsUndefined() && !uid_v->IsNull()) {
220216
CHECK(uid_v->IsInt32());
221217
const int32_t uid = uid_v.As<Int32>()->Value();
222218
options.flags |= UV_PROCESS_SETUID;
223219
options.uid = static_cast<uv_uid_t>(uid);
224220
}
225221

226-
// options.gid
227-
Local<Value> gid_v;
228-
if (!js_options->Get(context, env->gid_string()).ToLocal(&gid_v)) {
229-
return;
230-
}
222+
// args[7] gid
223+
Local<Value> gid_v = args[7];
231224
if (!gid_v->IsUndefined() && !gid_v->IsNull()) {
232225
CHECK(gid_v->IsInt32());
233226
const int32_t gid = gid_v.As<Int32>()->Value();
@@ -244,15 +237,11 @@ class ProcessWrap : public HandleWrap {
244237
err = UV_EINVAL;
245238
#endif
246239

247-
// options.args
248-
Local<Value> argv_v;
249-
if (!js_options->Get(context, env->args_string()).ToLocal(&argv_v)) {
250-
return;
251-
}
240+
// args[1] args
252241
std::vector<char*> options_args;
253242
std::vector<std::string> args_vals;
254-
if (argv_v->IsArray()) {
255-
Local<Array> js_argv = argv_v.As<Array>();
243+
if (args[1]->IsArray()) {
244+
Local<Array> js_argv = args[1].As<Array>();
256245
int argc = js_argv->Length();
257246
CHECK_LT(argc, INT_MAX); // Check for overflow.
258247
args_vals.reserve(argc);
@@ -273,26 +262,18 @@ class ProcessWrap : public HandleWrap {
273262
options.args = options_args.data();
274263
}
275264

276-
// options.cwd
277-
Local<Value> cwd_v;
278-
if (!js_options->Get(context, env->cwd_string()).ToLocal(&cwd_v)) {
279-
return;
280-
}
265+
// args[2] cwd
281266
node::Utf8Value cwd(env->isolate(),
282-
cwd_v->IsString() ? cwd_v : Local<Value>());
267+
args[2]->IsString() ? args[2] : Local<Value>());
283268
if (cwd.length() > 0) {
284269
options.cwd = *cwd;
285270
}
286271

287-
// options.env
288-
Local<Value> env_v;
289-
if (!js_options->Get(context, env->env_pairs_string()).ToLocal(&env_v)) {
290-
return;
291-
}
272+
// args[3] envPairs
292273
std::vector<char*> options_env;
293274
std::vector<std::string> env_vals;
294-
if (env_v->IsArray()) {
295-
Local<Array> env_opt = env_v.As<Array>();
275+
if (args[3]->IsArray()) {
276+
Local<Array> env_opt = args[3].As<Array>();
296277
int envc = env_opt->Length();
297278
CHECK_LT(envc, INT_MAX); // Check for overflow.
298279
env_vals.reserve(envc);
@@ -313,48 +294,31 @@ class ProcessWrap : public HandleWrap {
313294
options.env = options_env.data();
314295
}
315296

316-
// options.stdio
297+
// args[4] stdio
317298
std::vector<uv_stdio_container_t> options_stdio;
318-
if (ParseStdioOptions(env, js_options, &options_stdio).IsNothing()) {
299+
if (ParseStdioOptions(env, args[4], &options_stdio).IsNothing()) {
319300
return;
320301
}
321302
options.stdio = options_stdio.data();
322303
options.stdio_count = options_stdio.size();
323304

324-
// options.windowsHide
325-
Local<Value> hide_v;
326-
if (!js_options->Get(context, env->windows_hide_string())
327-
.ToLocal(&hide_v)) {
328-
return;
329-
}
305+
// args[5] flags (detached, windowsHide, windowsVerbatimArguments)
306+
CHECK(args[5]->IsUint32());
307+
const uint32_t flags = args[5].As<Uint32>()->Value();
330308

331-
if (hide_v->IsTrue()) {
309+
if (flags & kProcessFlagWindowsHide) {
332310
options.flags |= UV_PROCESS_WINDOWS_HIDE;
333311
}
334312

335313
if (env->hide_console_windows()) {
336314
options.flags |= UV_PROCESS_WINDOWS_HIDE_CONSOLE;
337315
}
338316

339-
// options.windows_verbatim_arguments
340-
Local<Value> wva_v;
341-
if (!js_options->Get(context, env->windows_verbatim_arguments_string())
342-
.ToLocal(&wva_v)) {
343-
return;
344-
}
345-
346-
if (wva_v->IsTrue()) {
317+
if (flags & kProcessFlagWindowsVerbatimArguments) {
347318
options.flags |= UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS;
348319
}
349320

350-
// options.detached
351-
Local<Value> detached_v;
352-
if (!js_options->Get(context, env->detached_string())
353-
.ToLocal(&detached_v)) {
354-
return;
355-
}
356-
357-
if (detached_v->IsTrue()) {
321+
if (flags & kProcessFlagDetached) {
358322
options.flags |= UV_PROCESS_DETACHED;
359323
}
360324

‎typings/globals.d.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import { MessagingBinding } from './internalBinding/messaging';
1818
import { OptionsBinding } from './internalBinding/options';
1919
import { OSBinding } from './internalBinding/os';
2020
import { ProcessBinding } from './internalBinding/process';
21+
import { ProcessWrapBinding } from './internalBinding/process_wrap';
2122
import { SeaBinding } from './internalBinding/sea';
2223
import { SerdesBinding } from './internalBinding/serdes';
2324
import { StringDecoderBinding } from './internalBinding/string_decoder';
@@ -55,6 +56,7 @@ interface InternalBindingMap {
5556
options: OptionsBinding;
5657
os: OSBinding;
5758
process: ProcessBinding;
59+
process_wrap: ProcessWrapBinding;
5860
sea: SeaBinding;
5961
serdes: SerdesBinding;
6062
string_decoder: StringDecoderBinding;

‎typings/internalBinding/process_wrap.d.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import { owner_symbol } from './symbols';
2+
3+
declare namespace InternalProcessWrapBinding {
4+
type StdioType = 'ignore' | 'pipe' | 'overlapped' | 'wrap' | 'inherit' | 'fd';
5+
6+
interface StdioContainer {
7+
type: StdioType;
8+
handle?: object;
9+
fd?: number;
10+
}
11+
12+
class Process {
13+
constructor();
14+
[owner_symbol]?: object;
15+
pid: number;
16+
onexit: (exitCode: number, signalCode: string) => void;
17+
spawn(
18+
file: string,
19+
args: string[] | undefined,
20+
cwd: string | undefined,
21+
envPairs: string[] | undefined,
22+
stdio: StdioContainer[],
23+
flags: number,
24+
uid: number | null | undefined,
25+
gid: number | null | undefined,
26+
): number;
27+
kill(signal: number): number;
28+
ref(): void;
29+
unref(): void;
30+
close(callback?: () => void): void;
31+
}
32+
33+
interface ProcessConstants {
34+
kProcessFlagDetached: number;
35+
kProcessFlagWindowsHide: number;
36+
kProcessFlagWindowsVerbatimArguments: number;
37+
}
38+
}
39+
40+
export interface ProcessWrapBinding {
41+
Process: typeof InternalProcessWrapBinding.Process;
42+
constants: InternalProcessWrapBinding.ProcessConstants;
43+
}

0 commit comments

Comments
 (0)