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

Conversation

joyeecheung
Copy link
Member

This PR removes

  • pushValueToArray
  • NODE_PUSH_VAL_TO_ARRAY_MAX
  • env->env->push_values_to_array_function()

In favor of the new V8 C++ API that constructs an Array from a C++ array directly.

Also removes setupProcessObject since by now it is only doing the push_values_to_array_function setup.
Also added a test for os.cpus() values.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 9, 2018
@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 9, 2018

Some local benchmark results:

                               confidence improvement accuracy (*)   (**)  (***)
 process/bench-env.js n=100000         **      1.99 %       ±1.39% ±1.84% ±2.40%

                    confidence improvement accuracy (*)   (**)  (***)
 os/cpus.js n=30000                -0.13 %       ±0.77% ±1.02% ±1.33%

                                        confidence improvement accuracy (*)   (**)  (***)
 http2/headers.js nheaders=0 n=1000                    0.33 %       ±1.06% ±1.40% ±1.83%
 http2/headers.js nheaders=10 n=1000                   0.44 %       ±1.03% ±1.37% ±1.79%
 http2/headers.js nheaders=100 n=1000                 -0.25 %       ±0.97% ±1.29% ±1.68%
 http2/headers.js nheaders=1000 n=1000         **      1.96 %       ±1.40% ±1.86% ±2.42%

The bench-parser results are rather flaky, and the benchmark is written in a strange way - it executes the same parser on the same payload in a hot loop, which almost never happen in the real world. When I turn the number of loop iteration down to 100 (probably the optimizing compiler doesn't kick in or doesn't do OSR doesn't optimize as aggressively that way) the improvement seems consistent, but I am not quite sure about that either. Then HTTP2 header parser that are migrated in the same way showed no significant impact, however, but that's benchmarked differently (it's not in a hot loop, instead it parse the headers sequentially in callbacks)

Intuitively the new API is doing what has to be done either in JS or C++ - it constructs an Array out of a FixedArray with all the elements readily packed inside, so it shouldn't be slower than the old way (which calls into JS), the impact seems to come from how the new approach affect what gets optimized in a hot loop which rarely happen in real word, so I am inclined to just ignore the results in http/bench-parser, but someone who has a better knowledge on the optimizing compiler may have a better insight than I am (cc @bmeurer ?).

For reference, something like #24125 shows significant improvement when the length of the array is long enough and the call cannot be optimized (e.g. unconditionally done in C++ land without kHasNoSideEffect notation)

From `node benchmark/compare.js --new ./node_new --old ./node_old --filter parser http`

                                       confidence improvement accuracy (*)   (**)   (***)
 http/bench-parser.js n=100000 len=16        ***    -13.15 %       ±7.09% ±9.44% ±12.29%
 http/bench-parser.js n=100000 len=32          *     -9.01 %       ±6.81% ±9.07% ±11.80%
 http/bench-parser.js n=100000 len=4         ***    -22.69 %       ±7.29% ±9.70% ±12.65%
 http/bench-parser.js n=100000 len=8         ***    -17.32 %       ±6.99% ±9.31% ±12.12%

Another time from `node benchmark/compare.js --new ./node_new --old ./node_old --filter parser http`

                                      confidence improvement accuracy (*)    (**)   (***)
 http/bench-parser.js n=100000 len=16               -12.83 %      ±17.71% ±23.56% ±30.67%
 http/bench-parser.js n=100000 len=32                -9.39 %      ±17.83% ±23.73% ±30.88%
 http/bench-parser.js n=100000 len=4           *    -21.00 %      ±17.30% ±23.04% ±30.01%
 http/bench-parser.js n=100000 len=8                -16.33 %      ±17.42% ±23.18% ±30.17%

Setting n=100:

                                   confidence improvement accuracy (*)   (**)  (***)
 http/bench-parser.js n=100 len=16        ***     27.89 %       ±2.98% ±3.98% ±5.19%
 http/bench-parser.js n=100 len=32        ***     40.82 %       ±2.65% ±3.56% ±4.70%
 http/bench-parser.js n=100 len=4           *      5.47 %       ±4.81% ±6.41% ±8.37%
 http/bench-parser.js n=100 len=8         ***     17.01 %       ±3.77% ±5.07% ±6.73%

Copy link
Member

@bmeurer bmeurer left a comment

Choose a reason for hiding this comment

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

Not sure about the benchmark results TBH.

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

Choose a reason for hiding this comment

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

Since you already know the capacity needed for this, how about pre-allocating it here?

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 idea! Thanks for catching that

fn->Call(env()->context(), headers, j * 2, argv).ToLocalChecked();
}
} while (i < num_values_);
std::vector<Local<Value>> headers_v;
Copy link
Member

Choose a reason for hiding this comment

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

Same here, how about preallocating the appropriate capacity?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now come to think of it, this one can actually be an array since its capacity cannot be bigger than 32*2, maybe that'll speed it up a bit more..

Copy link
Contributor

@refack refack Nov 11, 2018

Choose a reason for hiding this comment

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

I'd trust std::vector. No need to over-optimize. Are the benchmark around this? 😮

Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.
The usage of NODE_PUSH_VAL_TO_ARRAY_MAX and
push_values_to_array_function has all been removed
in favor of the new Array::New API that takes a C++
array. Remove the unused code.
@joyeecheung
Copy link
Member Author

Apparently using a fixed-size array in the http parser does the trick, though the impact shown from bench-parser.js is still influenced by the number of iterations, and the accuracy is still not too high when n=1e5 - I don't really know what to make out of this but I guess it doesn't really hurt ¯\(ツ)

                                      confidence improvement accuracy (*)   (**)  (***)
 http/bench-parser.js n=100000 len=16        ***     10.44 %       ±3.90% ±5.20% ±6.76%
 http/bench-parser.js n=100000 len=32        ***     11.36 %       ±3.78% ±5.04% ±6.59%
 http/bench-parser.js n=100000 len=4           *      6.81 %       ±5.39% ±7.17% ±9.33%
 http/bench-parser.js n=100000 len=8         ***     11.29 %       ±3.82% ±5.09% ±6.63%

when n=100 (somehow accuracy is a little bit better):

                                   confidence improvement accuracy (*)   (**)  (***)
 http/bench-parser.js n=100 len=16        ***     47.65 %       ±2.68% ±3.57% ±4.65%
 http/bench-parser.js n=100 len=32        ***     62.44 %       ±3.34% ±4.46% ±5.85%
 http/bench-parser.js n=100 len=4         ***     21.83 %       ±4.10% ±5.47% ±7.17%
 http/bench-parser.js n=100 len=8         ***     41.69 %       ±3.60% ±4.83% ±6.36%

Other benchmark results:

                               confidence improvement accuracy (*)   (**)  (***)
 process/bench-env.js n=100000         **      3.65 %       ±2.36% ±3.14% ±4.09%

                    confidence improvement accuracy (*)   (**)  (***)
 os/cpus.js n=30000                -0.27 %       ±2.07% ±2.76% ±3.59%

                                       confidence improvement accuracy (*)   (**)  (***)
 http2/headers.js nheaders=0 n=1000                   -0.42 %       ±2.30% ±3.06% ±3.99%
 http2/headers.js nheaders=10 n=1000                   0.89 %       ±2.10% ±2.80% ±3.65%
 http2/headers.js nheaders=100 n=1000                  1.00 %       ±2.16% ±2.87% ±3.74%
 http2/headers.js nheaders=1000 n=1000          *      1.61 %       ±1.55% ±2.07% ±2.69%

@joyeecheung
Copy link
Member Author

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

🎉
(just have some style nits)

lib/os.js Outdated Show resolved Hide resolved
src/node_http2.cc Show resolved Hide resolved
size_t headers_size = headers.size();
std::vector<Local<Value>> headers_v(headers_size * 2);
for (size_t i = 0; i < headers_size; ++i) {
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?

src/node_http2.cc Show resolved Hide resolved
src/node_http2.cc Show resolved Hide resolved
}

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

src/node_http_parser.cc Show resolved Hide resolved
src/node_os.cc Show resolved Hide resolved
lib/os.js Show resolved Hide resolved
lib/os.js Show resolved Hide resolved
@refack

This comment has been minimized.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Nice work!

And, to be explicit, I’m 👍 on using indexed for-loops here as well (for the reasons @joyeecheung is mentioning).

size_t headers_size = headers.size();
std::vector<Local<Value>> headers_v(headers_size * 2);
for (size_t i = 0; i < headers_size; ++i) {
nghttp2_header item = headers[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 think the compiler should be able optimize this away anyway?

if (j > 0)
fn->Call(context, holder, j, argv).ToLocalChecked();
for (size_t i = 0; i < nov; ++i) {
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 :)

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 13, 2018

Interesting results: I tried using reserve and then push_back or emplace_back in CreateHeaders (the hottest path in this patch), and they both cause a slight performance regression compared to assigning and indexing into the vector with high accuracy.

push_back:

                                      confidence improvement accuracy (*)   (**)  (***)
 http/bench-parser.js n=100000 len=16         **     -2.38 %       ±1.57% ±2.10% ±2.74%
 http/bench-parser.js n=100000 len=32        ***     -3.26 %       ±1.55% ±2.07% ±2.69%
 http/bench-parser.js n=100000 len=4         ***     -6.79 %       ±1.71% ±2.28% ±2.98%
 http/bench-parser.js n=100000 len=8         ***     -3.68 %       ±2.09% ±2.78% ±3.63%

emplace_back:

                                      confidence improvement accuracy (*)   (**)  (***)
 http/bench-parser.js n=100000 len=16                -0.92 %       ±1.81% ±2.41% ±3.13%
 http/bench-parser.js n=100000 len=32        ***     -5.43 %       ±2.19% ±2.92% ±3.80%
 http/bench-parser.js n=100000 len=4         ***     -4.89 %       ±1.88% ±2.52% ±3.29%
 http/bench-parser.js n=100000 len=8         ***     -2.76 %       ±1.58% ±2.11% ±2.75%

I suspect RVO is somehow not in effect with the way v8::Local<> is implemented/templated? Note that v8::Local<>'s default constructor merely initializes a pointer to 0 and the copy constructor doesn't do much either. But I prefer indexing into the vector with self-documenting equations anyway, so I didn't look into that any further.

Fixed a few nits with const references and a few comments. CI: https://ci.nodejs.org/job/node-test-pull-request/18576/

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 13, 2018
joyeecheung added a commit that referenced this pull request Nov 13, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
joyeecheung added a commit that referenced this pull request Nov 13, 2018
The usage of NODE_PUSH_VAL_TO_ARRAY_MAX and
push_values_to_array_function has all been removed
in favor of the new Array::New API that takes a C++
array. Remove the unused code.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
The usage of NODE_PUSH_VAL_TO_ARRAY_MAX and
push_values_to_array_function has all been removed
in favor of the new Array::New API that takes a C++
array. Remove the unused code.

PR-URL: #24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: nodejs#24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: nodejs#24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: nodejs#24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: nodejs#24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: nodejs#24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: nodejs#24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Instead of calling into JS from C++ to push values into an array,
use the new Array::New API that takes a pointer and a length
directly.

PR-URL: nodejs#24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
The usage of NODE_PUSH_VAL_TO_ARRAY_MAX and
push_values_to_array_function has all been removed
in favor of the new Array::New API that takes a C++
array. Remove the unused code.

PR-URL: nodejs#24264
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@targos targos mentioned this pull request Nov 28, 2018
2 tasks
@codebytere
Copy link
Member

@joyeecheung can/should this be backported to v10.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants