-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
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.
Review requested:
|
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using MaybeStackBuffer
to avoid allocations if number of elements is not too high?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think v8::Array
-> std::vector
would make sense (to avoid C++ -> JS invocation costs) but std::vector
-> MaybeStackBuffer
probably would not make much difference...(and personally I prefer STL unless the home-grown alternatives makes a significant difference)
Landed in a5ede9c |
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. PR-URL: #46447 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: James M Snell <jasnell@gmail.com>
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. PR-URL: #46447 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: James M Snell <jasnell@gmail.com>
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.