From efde153026ed48ae64413ecd5cf53fffcff1cac8 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Mon, 16 Oct 2017 20:26:10 -0400 Subject: [PATCH 1/3] src: increase usage of context in process_wrap.cc Refs: https://github.com/nodejs/node/pull/15380 PR-URL: https://github.com/nodejs/node/pull/16247 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/process_wrap.cc | 76 +++++++++++++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 27 deletions(-) diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 6613a571891a7f..d5a408af692111 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -93,16 +93,20 @@ class ProcessWrap : public HandleWrap { static void ParseStdioOptions(Environment* env, Local js_options, uv_process_options_t* options) { + Local context = env->context(); Local stdio_key = env->stdio_string(); - Local stdios = js_options->Get(stdio_key).As(); + Local stdios = + js_options->Get(context, stdio_key).ToLocalChecked().As(); uint32_t len = stdios->Length(); options->stdio = new uv_stdio_container_t[len]; options->stdio_count = len; for (uint32_t i = 0; i < len; i++) { - Local stdio = stdios->Get(i).As(); - Local type = stdio->Get(env->type_string()); + Local stdio = + stdios->Get(context, i).ToLocalChecked().As(); + Local type = + stdio->Get(context, env->type_string()).ToLocalChecked(); if (type->Equals(env->ignore_string())) { options->stdio[i].flags = UV_IGNORE; @@ -110,14 +114,16 @@ class ProcessWrap : public HandleWrap { options->stdio[i].flags = static_cast( UV_CREATE_PIPE | UV_READABLE_PIPE | UV_WRITABLE_PIPE); Local handle_key = env->handle_string(); - Local handle = stdio->Get(handle_key).As(); + Local handle = + stdio->Get(context, handle_key).ToLocalChecked().As(); CHECK(!handle.IsEmpty()); options->stdio[i].data.stream = reinterpret_cast( Unwrap(handle)->UVHandle()); } else if (type->Equals(env->wrap_string())) { Local handle_key = env->handle_string(); - Local handle = stdio->Get(handle_key).As(); + Local handle = + stdio->Get(context, handle_key).ToLocalChecked().As(); uv_stream_t* stream = HandleToStream(env, handle); CHECK_NE(stream, nullptr); @@ -125,7 +131,8 @@ class ProcessWrap : public HandleWrap { options->stdio[i].data.stream = stream; } else { Local fd_key = env->fd_string(); - int fd = static_cast(stdio->Get(fd_key)->IntegerValue()); + int fd = static_cast( + stdio->Get(context, fd_key).ToLocalChecked()->IntegerValue()); options->stdio[i].flags = UV_INHERIT_FD; options->stdio[i].data.fd = fd; } @@ -134,7 +141,7 @@ class ProcessWrap : public HandleWrap { static void Spawn(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - + Local context = env->context(); ProcessWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); @@ -146,19 +153,21 @@ class ProcessWrap : public HandleWrap { options.exit_cb = OnExit; // options.uid - Local uid_v = js_options->Get(env->uid_string()); + Local uid_v = + js_options->Get(context, env->uid_string()).ToLocalChecked(); if (!uid_v->IsUndefined() && !uid_v->IsNull()) { CHECK(uid_v->IsInt32()); - const int32_t uid = uid_v->Int32Value(env->context()).FromJust(); + const int32_t uid = uid_v->Int32Value(context).FromJust(); options.flags |= UV_PROCESS_SETUID; options.uid = static_cast(uid); } // options.gid - Local gid_v = js_options->Get(env->gid_string()); + Local gid_v = + js_options->Get(context, env->gid_string()).ToLocalChecked(); if (!gid_v->IsUndefined() && !gid_v->IsNull()) { CHECK(gid_v->IsInt32()); - const int32_t gid = gid_v->Int32Value(env->context()).FromJust(); + const int32_t gid = gid_v->Int32Value(context).FromJust(); options.flags |= UV_PROCESS_SETGID; options.gid = static_cast(gid); } @@ -166,20 +175,23 @@ class ProcessWrap : public HandleWrap { // TODO(bnoordhuis) is this possible to do without mallocing ? // options.file - Local file_v = js_options->Get(env->file_string()); + Local file_v = + js_options->Get(context, env->file_string()).ToLocalChecked(); CHECK(file_v->IsString()); node::Utf8Value file(env->isolate(), file_v); options.file = *file; // options.args - Local argv_v = js_options->Get(env->args_string()); + Local argv_v = + js_options->Get(context, env->args_string()).ToLocalChecked(); if (!argv_v.IsEmpty() && argv_v->IsArray()) { Local js_argv = Local::Cast(argv_v); int argc = js_argv->Length(); // Heap allocate to detect errors. +1 is for nullptr. options.args = new char*[argc + 1]; for (int i = 0; i < argc; i++) { - node::Utf8Value arg(env->isolate(), js_argv->Get(i)); + node::Utf8Value arg(env->isolate(), + js_argv->Get(context, i).ToLocalChecked()); options.args[i] = strdup(*arg); CHECK_NE(options.args[i], nullptr); } @@ -187,7 +199,8 @@ class ProcessWrap : public HandleWrap { } // options.cwd - Local cwd_v = js_options->Get(env->cwd_string()); + Local cwd_v = + js_options->Get(context, env->cwd_string()).ToLocalChecked(); node::Utf8Value cwd(env->isolate(), cwd_v->IsString() ? cwd_v : Local()); if (cwd.length() > 0) { @@ -195,13 +208,15 @@ class ProcessWrap : public HandleWrap { } // options.env - Local env_v = js_options->Get(env->env_pairs_string()); + Local env_v = + js_options->Get(context, env->env_pairs_string()).ToLocalChecked(); if (!env_v.IsEmpty() && env_v->IsArray()) { Local env_opt = Local::Cast(env_v); int envc = env_opt->Length(); options.env = new char*[envc + 1]; // Heap allocated to detect errors. for (int i = 0; i < envc; i++) { - node::Utf8Value pair(env->isolate(), env_opt->Get(i)); + node::Utf8Value pair(env->isolate(), + env_opt->Get(context, i).ToLocalChecked()); options.env[i] = strdup(*pair); CHECK_NE(options.env[i], nullptr); } @@ -212,22 +227,27 @@ class ProcessWrap : public HandleWrap { ParseStdioOptions(env, js_options, &options); // options.windowsHide - Local windows_hide_key = env->windows_hide_string(); + Local hide_v = + js_options->Get(context, env->windows_hide_string()).ToLocalChecked(); - if (js_options->Get(windows_hide_key)->IsTrue()) { + if (hide_v->IsTrue()) { options.flags |= UV_PROCESS_WINDOWS_HIDE; } // options.windows_verbatim_arguments - Local windows_verbatim_arguments_key = - env->windows_verbatim_arguments_string(); - if (js_options->Get(windows_verbatim_arguments_key)->IsTrue()) { + Local wva_v = + js_options->Get(context, env->windows_verbatim_arguments_string()) + .ToLocalChecked(); + + if (wva_v->IsTrue()) { options.flags |= UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS; } // options.detached - Local detached_key = env->detached_string(); - if (js_options->Get(detached_key)->IsTrue()) { + Local detached_v = + js_options->Get(context, env->detached_string()).ToLocalChecked(); + + if (detached_v->IsTrue()) { options.flags |= UV_PROCESS_DETACHED; } @@ -235,8 +255,9 @@ class ProcessWrap : public HandleWrap { if (err == 0) { CHECK_EQ(wrap->process_.data, wrap); - wrap->object()->Set(env->pid_string(), - Integer::New(env->isolate(), wrap->process_.pid)); + wrap->object()->Set(context, env->pid_string(), + Integer::New(env->isolate(), + wrap->process_.pid)).FromJust(); } if (options.args) { @@ -255,9 +276,10 @@ class ProcessWrap : public HandleWrap { } static void Kill(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); ProcessWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - int signal = args[0]->Int32Value(); + int signal = args[0]->Int32Value(env->context()).FromJust(); int err = uv_process_kill(&wrap->process_, signal); args.GetReturnValue().Set(err); } From ad0706551204667107d72b99e6cfc15bb95844fc Mon Sep 17 00:00:00 2001 From: cjihrig Date: Mon, 16 Oct 2017 23:48:15 -0400 Subject: [PATCH 2/3] src: increase usage of context in spawn_sync.cc Refs: https://github.com/nodejs/node/pull/15380 PR-URL: https://github.com/nodejs/node/pull/16247 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/spawn_sync.cc | 131 +++++++++++++++++++++++++++++----------------- 1 file changed, 84 insertions(+), 47 deletions(-) diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index c9765f51b267f4..24922ac8daa28c 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -663,39 +663,47 @@ void SyncProcessRunner::SetPipeError(int pipe_error) { Local SyncProcessRunner::BuildResultObject() { EscapableHandleScope scope(env()->isolate()); + Local context = env()->context(); Local js_result = Object::New(env()->isolate()); if (GetError() != 0) { - js_result->Set(env()->error_string(), - Integer::New(env()->isolate(), GetError())); + js_result->Set(context, env()->error_string(), + Integer::New(env()->isolate(), GetError())).FromJust(); } if (exit_status_ >= 0) { if (term_signal_ > 0) { - js_result->Set(env()->status_string(), Null(env()->isolate())); + js_result->Set(context, env()->status_string(), + Null(env()->isolate())).FromJust(); } else { - js_result->Set(env()->status_string(), - Number::New(env()->isolate(), static_cast(exit_status_))); + js_result->Set(context, env()->status_string(), + Number::New(env()->isolate(), + static_cast(exit_status_))).FromJust(); } } else { // If exit_status_ < 0 the process was never started because of some error. - js_result->Set(env()->status_string(), Null(env()->isolate())); + js_result->Set(context, env()->status_string(), + Null(env()->isolate())).FromJust(); } if (term_signal_ > 0) - js_result->Set(env()->signal_string(), - String::NewFromUtf8(env()->isolate(), signo_string(term_signal_))); + js_result->Set(context, env()->signal_string(), + String::NewFromUtf8(env()->isolate(), + signo_string(term_signal_))).FromJust(); else - js_result->Set(env()->signal_string(), Null(env()->isolate())); + js_result->Set(context, env()->signal_string(), + Null(env()->isolate())).FromJust(); if (exit_status_ >= 0) - js_result->Set(env()->output_string(), BuildOutputArray()); + js_result->Set(context, env()->output_string(), + BuildOutputArray()).FromJust(); else - js_result->Set(env()->output_string(), Null(env()->isolate())); + js_result->Set(context, env()->output_string(), + Null(env()->isolate())).FromJust(); - js_result->Set(env()->pid_string(), - Number::New(env()->isolate(), uv_process_.pid)); + js_result->Set(context, env()->pid_string(), + Number::New(env()->isolate(), uv_process_.pid)).FromJust(); return scope.Escape(js_result); } @@ -706,14 +714,15 @@ Local SyncProcessRunner::BuildOutputArray() { CHECK_NE(stdio_pipes_, nullptr); EscapableHandleScope scope(env()->isolate()); + Local context = env()->context(); Local js_output = Array::New(env()->isolate(), stdio_count_); for (uint32_t i = 0; i < stdio_count_; i++) { SyncProcessStdioPipe* h = stdio_pipes_[i]; if (h != nullptr && h->writable()) - js_output->Set(i, h->GetOutputAsBuffer(env())); + js_output->Set(context, i, h->GetOutputAsBuffer(env())).FromJust(); else - js_output->Set(i, Null(env()->isolate())); + js_output->Set(context, i, Null(env()->isolate())).FromJust(); } return scope.Escape(js_output); @@ -727,22 +736,25 @@ int SyncProcessRunner::ParseOptions(Local js_value) { if (!js_value->IsObject()) return UV_EINVAL; + Local context = env()->context(); Local js_options = js_value.As(); - Local js_file = js_options->Get(env()->file_string()); + Local js_file = + js_options->Get(context, env()->file_string()).ToLocalChecked(); r = CopyJsString(js_file, &file_buffer_); if (r < 0) return r; uv_process_options_.file = file_buffer_; - Local js_args = js_options->Get(env()->args_string()); + Local js_args = + js_options->Get(context, env()->args_string()).ToLocalChecked(); r = CopyJsStringArray(js_args, &args_buffer_); if (r < 0) return r; uv_process_options_.args = reinterpret_cast(args_buffer_); - - Local js_cwd = js_options->Get(env()->cwd_string()); + Local js_cwd = + js_options->Get(context, env()->cwd_string()).ToLocalChecked(); if (IsSet(js_cwd)) { r = CopyJsString(js_cwd, &cwd_buffer_); if (r < 0) @@ -750,7 +762,8 @@ int SyncProcessRunner::ParseOptions(Local js_value) { uv_process_options_.cwd = cwd_buffer_; } - Local js_env_pairs = js_options->Get(env()->env_pairs_string()); + Local js_env_pairs = + js_options->Get(context, env()->env_pairs_string()).ToLocalChecked(); if (IsSet(js_env_pairs)) { r = CopyJsStringArray(js_env_pairs, &env_buffer_); if (r < 0) @@ -758,55 +771,65 @@ int SyncProcessRunner::ParseOptions(Local js_value) { uv_process_options_.env = reinterpret_cast(env_buffer_); } - Local js_uid = js_options->Get(env()->uid_string()); + Local js_uid = + js_options->Get(context, env()->uid_string()).ToLocalChecked(); if (IsSet(js_uid)) { CHECK(js_uid->IsInt32()); - const int32_t uid = js_uid->Int32Value(env()->context()).FromJust(); + const int32_t uid = js_uid->Int32Value(context).FromJust(); uv_process_options_.uid = static_cast(uid); uv_process_options_.flags |= UV_PROCESS_SETUID; } - Local js_gid = js_options->Get(env()->gid_string()); + Local js_gid = + js_options->Get(context, env()->gid_string()).ToLocalChecked(); if (IsSet(js_gid)) { CHECK(js_gid->IsInt32()); - const int32_t gid = js_gid->Int32Value(env()->context()).FromJust(); + const int32_t gid = js_gid->Int32Value(context).FromJust(); uv_process_options_.gid = static_cast(gid); uv_process_options_.flags |= UV_PROCESS_SETGID; } - if (js_options->Get(env()->detached_string())->BooleanValue()) + Local js_detached = + js_options->Get(context, env()->detached_string()).ToLocalChecked(); + if (js_detached->BooleanValue(context).FromJust()) uv_process_options_.flags |= UV_PROCESS_DETACHED; - Local win_hide = env()->windows_hide_string(); - - if (js_options->Get(win_hide)->BooleanValue()) + Local js_win_hide = + js_options->Get(context, env()->windows_hide_string()).ToLocalChecked(); + if (js_win_hide->BooleanValue(context).FromJust()) uv_process_options_.flags |= UV_PROCESS_WINDOWS_HIDE; - Local wba = env()->windows_verbatim_arguments_string(); + Local js_wva = + js_options->Get(context, env()->windows_verbatim_arguments_string()) + .ToLocalChecked(); - if (js_options->Get(wba)->BooleanValue()) + if (js_wva->BooleanValue(context).FromJust()) uv_process_options_.flags |= UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS; - Local js_timeout = js_options->Get(env()->timeout_string()); + Local js_timeout = + js_options->Get(context, env()->timeout_string()).ToLocalChecked(); if (IsSet(js_timeout)) { CHECK(js_timeout->IsNumber()); - int64_t timeout = js_timeout->IntegerValue(); + int64_t timeout = js_timeout->IntegerValue(context).FromJust(); timeout_ = static_cast(timeout); } - Local js_max_buffer = js_options->Get(env()->max_buffer_string()); + Local js_max_buffer = + js_options->Get(context, env()->max_buffer_string()).ToLocalChecked(); if (IsSet(js_max_buffer)) { CHECK(js_max_buffer->IsNumber()); - max_buffer_ = js_max_buffer->NumberValue(); + max_buffer_ = js_max_buffer->NumberValue(context).FromJust(); } - Local js_kill_signal = js_options->Get(env()->kill_signal_string()); + Local js_kill_signal = + js_options->Get(context, env()->kill_signal_string()).ToLocalChecked(); if (IsSet(js_kill_signal)) { CHECK(js_kill_signal->IsInt32()); - kill_signal_ = js_kill_signal->Int32Value(); + kill_signal_ = js_kill_signal->Int32Value(context).FromJust(); } - Local js_stdio = js_options->Get(env()->stdio_string()); + Local js_stdio = + js_options->Get(context, env()->stdio_string()).ToLocalChecked(); r = ParseStdioOptions(js_stdio); if (r < 0) return r; @@ -822,6 +845,7 @@ int SyncProcessRunner::ParseStdioOptions(Local js_value) { if (!js_value->IsArray()) return UV_EINVAL; + Local context = env()->context(); js_stdio_options = js_value.As(); stdio_count_ = js_stdio_options->Length(); @@ -831,7 +855,8 @@ int SyncProcessRunner::ParseStdioOptions(Local js_value) { stdio_pipes_initialized_ = true; for (uint32_t i = 0; i < stdio_count_; i++) { - Local js_stdio_option = js_stdio_options->Get(i); + Local js_stdio_option = + js_stdio_options->Get(context, i).ToLocalChecked(); if (!js_stdio_option->IsObject()) return UV_EINVAL; @@ -850,7 +875,9 @@ int SyncProcessRunner::ParseStdioOptions(Local js_value) { int SyncProcessRunner::ParseStdioOption(int child_fd, Local js_stdio_option) { - Local js_type = js_stdio_option->Get(env()->type_string()); + Local context = env()->context(); + Local js_type = + js_stdio_option->Get(context, env()->type_string()).ToLocalChecked(); if (js_type->StrictEquals(env()->ignore_string())) { return AddStdioIgnore(child_fd); @@ -859,13 +886,17 @@ int SyncProcessRunner::ParseStdioOption(int child_fd, Local rs = env()->readable_string(); Local ws = env()->writable_string(); - bool readable = js_stdio_option->Get(rs)->BooleanValue(); - bool writable = js_stdio_option->Get(ws)->BooleanValue(); + bool readable = js_stdio_option->Get(context, rs) + .ToLocalChecked()->BooleanValue(context).FromJust(); + bool writable = + js_stdio_option->Get(context, ws) + .ToLocalChecked()->BooleanValue(context).FromJust(); uv_buf_t buf = uv_buf_init(nullptr, 0); if (readable) { - Local input = js_stdio_option->Get(env()->input_string()); + Local input = + js_stdio_option->Get(context, env()->input_string()).ToLocalChecked(); if (Buffer::HasInstance(input)) { buf = uv_buf_init(Buffer::Data(input), static_cast(Buffer::Length(input))); @@ -881,7 +912,8 @@ int SyncProcessRunner::ParseStdioOption(int child_fd, } else if (js_type->StrictEquals(env()->inherit_string()) || js_type->StrictEquals(env()->fd_string())) { - int inherit_fd = js_stdio_option->Get(env()->fd_string())->Int32Value(); + int inherit_fd = js_stdio_option->Get(context, env()->fd_string()) + .ToLocalChecked()->Int32Value(context).FromJust(); return AddStdioInheritFD(child_fd, inherit_fd); } else { @@ -981,14 +1013,17 @@ int SyncProcessRunner::CopyJsStringArray(Local js_value, if (!js_value->IsArray()) return UV_EINVAL; + Local context = env()->context(); js_array = js_value.As()->Clone().As(); length = js_array->Length(); // Convert all array elements to string. Modify the js object itself if // needed - it's okay since we cloned the original object. for (uint32_t i = 0; i < length; i++) { - if (!js_array->Get(i)->IsString()) - js_array->Set(i, js_array->Get(i)->ToString(env()->isolate())); + auto value = js_array->Get(context, i).ToLocalChecked(); + + if (!value->IsString()) + js_array->Set(context, i, value->ToString(env()->isolate())).FromJust(); } // Index has a pointer to every string element, plus one more for a final @@ -999,7 +1034,8 @@ int SyncProcessRunner::CopyJsStringArray(Local js_value, // after every string. Align strings to cache lines. data_size = 0; for (uint32_t i = 0; i < length; i++) { - data_size += StringBytes::StorageSize(isolate, js_array->Get(i), UTF8) + 1; + auto value = js_array->Get(context, i).ToLocalChecked(); + data_size += StringBytes::StorageSize(isolate, value, UTF8) + 1; data_size = ROUND_UP(data_size, sizeof(void*)); } @@ -1010,10 +1046,11 @@ int SyncProcessRunner::CopyJsStringArray(Local js_value, for (uint32_t i = 0; i < length; i++) { list[i] = buffer + data_offset; + auto value = js_array->Get(context, i).ToLocalChecked(); data_offset += StringBytes::Write(isolate, buffer + data_offset, -1, - js_array->Get(i), + value, UTF8); buffer[data_offset++] = '\0'; data_offset = ROUND_UP(data_offset, sizeof(void*)); From 6dcc37d0ed974325f2048660dd641d4de3616be1 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Wed, 18 Oct 2017 13:59:52 -0400 Subject: [PATCH 3/3] src: combine loops in CopyJsStringArray() In spawn_sync.cc, two consecutive loops are used to convert data to strings, and compute the size of the data. This commit merges the two independent loops into one. Refs: https://github.com/nodejs/node/pull/15380 PR-URL: https://github.com/nodejs/node/pull/16247 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- src/spawn_sync.cc | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index 24922ac8daa28c..626ecbb04ff5b7 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -1016,25 +1016,22 @@ int SyncProcessRunner::CopyJsStringArray(Local js_value, Local context = env()->context(); js_array = js_value.As()->Clone().As(); length = js_array->Length(); + data_size = 0; + + // Index has a pointer to every string element, plus one more for a final + // null pointer. + list_size = (length + 1) * sizeof *list; // Convert all array elements to string. Modify the js object itself if - // needed - it's okay since we cloned the original object. + // needed - it's okay since we cloned the original object. Also compute the + // length of all strings, including room for a null terminator after every + // string. Align strings to cache lines. for (uint32_t i = 0; i < length; i++) { auto value = js_array->Get(context, i).ToLocalChecked(); if (!value->IsString()) js_array->Set(context, i, value->ToString(env()->isolate())).FromJust(); - } - // Index has a pointer to every string element, plus one more for a final - // null pointer. - list_size = (length + 1) * sizeof *list; - - // Compute the length of all strings. Include room for null terminator - // after every string. Align strings to cache lines. - data_size = 0; - for (uint32_t i = 0; i < length; i++) { - auto value = js_array->Get(context, i).ToLocalChecked(); data_size += StringBytes::StorageSize(isolate, value, UTF8) + 1; data_size = ROUND_UP(data_size, sizeof(void*)); }