Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: remove pushValueToArray and setupProcessObject #24264

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
// avoid retaining a reference to the bootstrap
// object.
{ _setupTraceCategoryState,
_setupProcessObject, _setupNextTick,
_setupNextTick,
_setupPromises, _chdir, _cpuUsage,
_hrtime, _hrtimeBigInt,
_memoryUsage, _rawDebug,
Expand Down Expand Up @@ -376,13 +376,6 @@
const origProcProto = Object.getPrototypeOf(process);
Object.setPrototypeOf(origProcProto, EventEmitter.prototype);
EventEmitter.call(process);

_setupProcessObject(pushValueToArray);

function pushValueToArray() {
for (var i = 0; i < arguments.length; i++)
this.push(arguments[i]);
}
}

function setupGlobalVariables() {
Expand Down
32 changes: 15 additions & 17 deletions lib/os.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

'use strict';

const { pushValToArrayMax, safeGetenv } = internalBinding('util');
const { safeGetenv } = internalBinding('util');
const constants = internalBinding('constants').os;
const { deprecate } = require('internal/util');
const isWindows = process.platform === 'win32';
Expand Down Expand Up @@ -82,31 +82,29 @@ const getNetworkInterfacesDepMsg =
'os.getNetworkInterfaces is deprecated. Use os.networkInterfaces instead.';

const avgValues = new Float64Array(3);
const cpuValues = new Float64Array(6 * pushValToArrayMax);

function loadavg() {
getLoadAvg(avgValues);
return [avgValues[0], avgValues[1], avgValues[2]];
}

function addCPUInfo() {
for (var i = 0, c = 0; i < arguments.length; ++i, c += 6) {
this[this.length] = {
model: arguments[i],
speed: cpuValues[c],
function cpus() {
const data = getCPUs();
const result = [];
refack marked this conversation as resolved.
Show resolved Hide resolved
for (var i = 0; i < data.length; i += 7) {
refack marked this conversation as resolved.
Show resolved Hide resolved
result.push({
model: data[i],
speed: data[i + 1],
times: {
user: cpuValues[c + 1],
nice: cpuValues[c + 2],
sys: cpuValues[c + 3],
idle: cpuValues[c + 4],
irq: cpuValues[c + 5]
user: data[i + 2],
nice: data[i + 3],
sys: data[i + 4],
idle: data[i + 5],
irq: data[i + 6]
}
};
});
}
}

function cpus() {
return getCPUs(addCPUInfo, cpuValues, []);
return result;
}

function arch() {
Expand Down
7 changes: 0 additions & 7 deletions src/bootstrapper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,6 @@ using v8::PromiseRejectMessage;
using v8::String;
using v8::Value;

void SetupProcessObject(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK(args[0]->IsFunction());
env->set_push_values_to_array_function(args[0].As<Function>());
}

void RunMicrotasks(const FunctionCallbackInfo<Value>& args) {
args.GetIsolate()->RunMicrotasks();
}
Expand Down Expand Up @@ -142,7 +136,6 @@ void SetupPromises(const FunctionCallbackInfo<Value>& args) {
void SetupBootstrapObject(Environment* env,
Local<Object> bootstrapper) {
BOOTSTRAP_METHOD(_setupTraceCategoryState, SetupTraceCategoryState);
BOOTSTRAP_METHOD(_setupProcessObject, SetupProcessObject);
BOOTSTRAP_METHOD(_setupNextTick, SetupNextTick);
BOOTSTRAP_METHOD(_setupPromises, SetupPromises);
BOOTSTRAP_METHOD(_chdir, Chdir);
Expand Down
5 changes: 0 additions & 5 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,6 @@ struct PackageConfig {
};
} // namespace loader

// The number of items passed to push_values_to_array_function has diminishing
// returns around 8. This should be used at all call sites using said function.
constexpr size_t NODE_PUSH_VAL_TO_ARRAY_MAX = 8;

// Stat fields buffers contain twice the number of entries in an uv_stat_t
// because `fs.StatWatcher` needs room to store 2 `fs.Stats` instances.
constexpr size_t kFsStatsFieldsNumber = 14;
Expand Down Expand Up @@ -353,7 +349,6 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
V(process_object, v8::Object) \
V(promise_handler_function, v8::Function) \
V(promise_wrap_template, v8::ObjectTemplate) \
V(push_values_to_array_function, v8::Function) \
V(sab_lifetimepartner_constructor_template, v8::FunctionTemplate) \
V(script_context_constructor_template, v8::FunctionTemplate) \
V(script_data_constructor_function, v8::Function) \
Expand Down
69 changes: 23 additions & 46 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1292,45 +1292,30 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
Local<String> value_str;

Local<Array> holder = Array::New(isolate);
Local<Function> fn = env()->push_values_to_array_function();
Local<Value> argv[NODE_PUSH_VAL_TO_ARRAY_MAX * 2];

// The headers are passed in above as a queue of nghttp2_header structs.
// The following converts that into a JS array with the structure:
// [name1, value1, name2, value2, name3, value3, name3, value4] and so on.
// That array is passed up to the JS layer and converted into an Object form
// like {name1: value1, name2: value2, name3: [value3, value4]}. We do it
// this way for performance reasons (it's faster to generate and pass an
// array than it is to generate and pass the object).
size_t n = 0;
while (n < headers.size()) {
size_t j = 0;
while (n < headers.size() && j < arraysize(argv) / 2) {
nghttp2_header item = headers[n++];
// The header name and value are passed as external one-byte strings
name_str =
ExternalHeader::New<true>(this, item.name).ToLocalChecked();
value_str =
ExternalHeader::New<false>(this, item.value).ToLocalChecked();
argv[j * 2] = name_str;
argv[j * 2 + 1] = value_str;
j++;
}
// For performance, we pass name and value pairs to array.protototype.push
// in batches of size NODE_PUSH_VAL_TO_ARRAY_MAX * 2 until there are no
// more items to push.
if (j > 0) {
fn->Call(env()->context(), holder, j * 2, argv).ToLocalChecked();
}
size_t headers_size = headers.size();
std::vector<Local<Value>> headers_v(headers_size * 2);
for (size_t i = 0; i < headers_size; ++i) {
refack marked this conversation as resolved.
Show resolved Hide resolved
nghttp2_header item = headers[i];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: this is an unneeded copy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! (though I realize it's originally that way :/)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the compiler should be able optimize this away anyway?

// The header name and value are passed as external one-byte strings
headers_v[i * 2] =
ExternalHeader::New<true>(this, item.name).ToLocalChecked();
headers_v[i * 2 + 1] =
ExternalHeader::New<false>(this, item.value).ToLocalChecked();
}

Local<Value> args[5] = {
refack marked this conversation as resolved.
Show resolved Hide resolved
stream->object(),
Integer::New(isolate, id),
Integer::New(isolate, stream->headers_category()),
Integer::New(isolate, frame->hd.flags),
holder
};
stream->object(),
Integer::New(isolate, id),
Integer::New(isolate, stream->headers_category()),
Integer::New(isolate, frame->hd.flags),
Array::New(isolate, headers_v.data(), headers_size * 2)};
MakeCallback(env()->onheaders_string(), arraysize(args), args);
}

Expand Down Expand Up @@ -1439,25 +1424,17 @@ void Http2Session::HandleOriginFrame(const nghttp2_frame* frame) {
nghttp2_extension ext = frame->ext;
nghttp2_ext_origin* origin = static_cast<nghttp2_ext_origin*>(ext.payload);

Local<Value> holder = Array::New(isolate);
Local<Function> fn = env()->push_values_to_array_function();
Local<Value> argv[NODE_PUSH_VAL_TO_ARRAY_MAX];
size_t nov = origin->nov;
std::vector<Local<Value>> origin_v(nov);

size_t n = 0;
while (n < origin->nov) {
size_t j = 0;
while (n < origin->nov && j < arraysize(argv)) {
auto entry = origin->ov[n++];
argv[j++] =
String::NewFromOneByte(isolate,
entry.origin,
v8::NewStringType::kNormal,
entry.origin_len).ToLocalChecked();
}
if (j > 0)
fn->Call(context, holder, j, argv).ToLocalChecked();
for (size_t i = 0; i < nov; ++i) {
refack marked this conversation as resolved.
Show resolved Hide resolved
auto entry = origin->ov[i];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is copy-pasted, but I think getting rid of this auto might make things more readable :)

origin_v[i] =
String::NewFromOneByte(
isolate, entry.origin, v8::NewStringType::kNormal, entry.origin_len)
.ToLocalChecked();
}

Local<Value> holder = Array::New(isolate, origin_v.data(), nov);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/s/nov/origin_v.size()/ for correctness and readability

MakeCallback(env()->onorigin_string(), 1, &holder);
}

Expand Down
37 changes: 14 additions & 23 deletions src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ const uint32_t kOnHeadersComplete = 1;
const uint32_t kOnBody = 2;
const uint32_t kOnMessageComplete = 3;
const uint32_t kOnExecute = 4;

// Any more fields than this will be flushed into JS
const size_t kMaxHeaderFieldsCount = 32;

// helper class for the Parser
struct StringPtr {
Expand Down Expand Up @@ -203,7 +204,7 @@ class Parser : public AsyncWrap, public StreamListener {
if (num_fields_ == num_values_) {
// start of new field name
num_fields_++;
if (num_fields_ == arraysize(fields_)) {
if (num_fields_ == kMaxHeaderFieldsCount) {
// ran out of space - flush to javascript land
Flush();
num_fields_ = 1;
Expand All @@ -212,7 +213,7 @@ class Parser : public AsyncWrap, public StreamListener {
fields_[num_fields_ - 1].Reset();
}

CHECK_LT(num_fields_, arraysize(fields_));
CHECK_LT(num_fields_, kMaxHeaderFieldsCount);
CHECK_EQ(num_fields_, num_values_ + 1);

fields_[num_fields_ - 1].Update(at, length);
Expand Down Expand Up @@ -731,25 +732,15 @@ class Parser : public AsyncWrap, public StreamListener {
}

Local<Array> CreateHeaders() {
Local<Array> headers = Array::New(env()->isolate());
Local<Function> fn = env()->push_values_to_array_function();
Local<Value> argv[NODE_PUSH_VAL_TO_ARRAY_MAX * 2];
size_t i = 0;

do {
size_t j = 0;
while (i < num_values_ && j < arraysize(argv) / 2) {
argv[j * 2] = fields_[i].ToString(env());
argv[j * 2 + 1] = values_[i].ToString(env());
i++;
j++;
}
if (j > 0) {
fn->Call(env()->context(), headers, j * 2, argv).ToLocalChecked();
}
} while (i < num_values_);
// There could be extra entries but the max size should be fixed
Local<Value> headers_v[kMaxHeaderFieldsCount * 2];

for (size_t i = 0; i < num_values_; ++i) {
refack marked this conversation as resolved.
Show resolved Hide resolved
headers_v[i * 2] = fields_[i].ToString(env());
headers_v[i * 2 + 1] = values_[i].ToString(env());
}

return headers;
return Array::New(env()->isolate(), headers_v, num_values_ * 2);
}


Expand Down Expand Up @@ -824,8 +815,8 @@ class Parser : public AsyncWrap, public StreamListener {
}

parser_t parser_;
StringPtr fields_[32]; // header fields
StringPtr values_[32]; // header values
StringPtr fields_[kMaxHeaderFieldsCount]; // header fields
StringPtr values_[kMaxHeaderFieldsCount]; // header values
StringPtr url_;
StringPtr status_message_;
size_t num_fields_;
Expand Down
50 changes: 14 additions & 36 deletions src/node_os.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ using v8::Function;
using v8::FunctionCallbackInfo;
using v8::Int32;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::MaybeLocal;
using v8::Null;
Expand Down Expand Up @@ -148,52 +149,29 @@ static void GetOSRelease(const FunctionCallbackInfo<Value>& args) {

static void GetCPUInfo(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();

uv_cpu_info_t* cpu_infos;
int count, i, field_idx;
int count;

int err = uv_cpu_info(&cpu_infos, &count);
if (err)
return;

CHECK(args[0]->IsFunction());
Local<Function> addfn = args[0].As<Function>();

CHECK(args[1]->IsFloat64Array());
Local<Float64Array> array = args[1].As<Float64Array>();
CHECK_EQ(array->Length(), 6 * NODE_PUSH_VAL_TO_ARRAY_MAX);
Local<ArrayBuffer> ab = array->Buffer();
double* fields = static_cast<double*>(ab->GetContents().Data());

CHECK(args[2]->IsArray());
Local<Array> cpus = args[2].As<Array>();

Local<Value> model_argv[NODE_PUSH_VAL_TO_ARRAY_MAX];
unsigned int model_idx = 0;

for (i = 0, field_idx = 0; i < count; i++) {
std::vector<Local<Value>> result(count * 7);
for (size_t i = 0; i < count; i++) {
refack marked this conversation as resolved.
Show resolved Hide resolved
uv_cpu_info_t* ci = cpu_infos + i;

fields[field_idx++] = ci->speed;
fields[field_idx++] = ci->cpu_times.user;
fields[field_idx++] = ci->cpu_times.nice;
fields[field_idx++] = ci->cpu_times.sys;
fields[field_idx++] = ci->cpu_times.idle;
fields[field_idx++] = ci->cpu_times.irq;
model_argv[model_idx++] = OneByteString(env->isolate(), ci->model);

if (model_idx >= NODE_PUSH_VAL_TO_ARRAY_MAX) {
addfn->Call(env->context(), cpus, model_idx, model_argv).ToLocalChecked();
model_idx = 0;
field_idx = 0;
}
}

if (model_idx > 0) {
addfn->Call(env->context(), cpus, model_idx, model_argv).ToLocalChecked();
result[i * 7] = OneByteString(isolate, ci->model);
result[i * 7 + 1] = Number::New(isolate, ci->speed);
result[i * 7 + 2] = Number::New(isolate, ci->cpu_times.user);
result[i * 7 + 3] = Number::New(isolate, ci->cpu_times.nice);
result[i * 7 + 4] = Number::New(isolate, ci->cpu_times.sys);
result[i * 7 + 5] = Number::New(isolate, ci->cpu_times.idle);
result[i * 7 + 6] = Number::New(isolate, ci->cpu_times.irq);
}

uv_free_cpu_info(cpu_infos, count);
args.GetReturnValue().Set(cpus);
args.GetReturnValue().Set(Array::New(isolate, result.data(), result.size()));
}


Expand Down
Loading