The Wayback Machine - https://web.archive.org/web/20230131200320/https://github.com/nodejs/node/pull/46447
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

http: use v8::Array::New() with a prebuilt vector #46447

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joyeecheung
Copy link
Member

Avoid using v8::Array::Set() which results in JS execution and is thus slow. Prebuild the vector in C++ land and build the JS array directly with that vector whereever possible.

Avoid using v8::Array::Set() which results in JS execution and is
thus slow. Prebuild the vector in C++ land and build the
JS array directly with that vector whereever possible.
@nodejs-github-bot
Copy link
Contributor

Review requested:

  • @nodejs/http
  • @nodejs/net
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run. labels Jan 31, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

ConnectionsList* list;

ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder());

uint32_t i = 0;
std::vector<Local<Value>> result;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::vector<Local<Value>> result;
std::vector<Local<Value>> result;
result.reserve(list->all_connections_.size());
ConnectionsList* list;

ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder());

uint32_t i = 0;
std::vector<Local<Value>> result;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::vector<Local<Value>> result;
std::vector<Local<Value>> result;
result.reserve(list->all_connections_.size());
ConnectionsList* list;

ASSIGN_OR_RETURN_UNWRAP(&list, args.Holder());

uint32_t i = 0;
std::vector<Local<Value>> result;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::vector<Local<Value>> result;
std::vector<Local<Value>> result;
result.reserve(list->all_connections_.size());
auto iter = list->active_connections_.begin();
auto end = list->active_connections_.end();

std::vector<Local<Value>> result;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::vector<Local<Value>> result;
std::vector<Local<Value>> result;
result.reserve(list->all_connections_.size());
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run.
5 participants