src: replace pushValueToArray with pure C++ API by joyeecheung · Pull Request #24125 · nodejs/node

joyeecheung · 2018-11-06T11:24:52Z

The first commit just landed upstream: https://chromium-review.googlesource.com/c/1317049
The second commit replaces the pushValueToArray hack with the new V8 API in fs.readdir and fs.readdirSync.
The third commit added more options to the readdir benchmark.

Local benchmark results:

                                                                        confidence improvement accuracy (*)   (**)  (***)
 fs/bench-readdir.js withFileTypes='false' dir='lib' n=10                              -0.33 %       ±4.75% ±6.33% ±8.26%
 fs/bench-readdir.js withFileTypes='false' dir='test/parallel' n=10            ***      8.96 %       ±2.83% ±3.78% ±4.93%
 fs/bench-readdir.js withFileTypes='true' dir='lib' n=10                                0.17 %       ±4.36% ±5.80% ±7.56%
 fs/bench-readdir.js withFileTypes='true' dir='test/parallel' n=10                      0.53 %       ±2.37% ±3.16% ±4.11%
 fs/bench-readdirSync.js withFileTypes='false' dir='lib' n=10                  ***      6.21 %       ±3.41% ±4.56% ±5.97%
 fs/bench-readdirSync.js withFileTypes='false' dir='test/parallel' n=10        ***     13.29 %       ±2.35% ±3.13% ±4.08%
 fs/bench-readdirSync.js withFileTypes='true' dir='lib' n=10                   ***      6.27 %       ±1.81% ±2.41% ±3.15%
 fs/bench-readdirSync.js withFileTypes='true' dir='test/parallel' n=10         ***      9.88 %       ±2.51% ±3.34% ±4.35%

With this I hope we can remove all the push_values_to_array_function usage and get rid of SetupProcessObject eventually - that can be done in later PRs, of course.

deps: cherry-pick `0483e9a` from upstream V8

Original commit message:

[api] Allow embedder to construct an Array from Local<Value>*

Currently to obtain a v8::Array out of a C array or a std::vector,
one needs to loop through the elements and call array->Set() multiple
times, and these calls go into v8::Object::Set() which can be slow.
This patch adds a new Array::New overload that converts a
Local<Value>* with known size into a Local<Array>.

Change-Id: I0a768f0e18eec51e78d58be455482ec6425ca188
Reviewed-on: https://chromium-review.googlesource.com/c/1317049
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/master@{#57261}

Refs: v8/v8@0483e9a

fs: replace pushValueToArray with pure C++ API

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.

benchmark: add dir and withFileTypes option readdir benchmarks

Checklist

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

Original commit message: [api] Allow embedder to construct an Array from Local<Value>* Currently to obtain a v8::Array out of a C array or a std::vector, one needs to loop through the elements and call array->Set() multiple times, and these calls go into v8::Object::Set() which can be slow. This patch adds a new Array::New overload that converts a Local<Value>* with known size into a Local<Array>. Change-Id: I0a768f0e18eec51e78d58be455482ec6425ca188 Reviewed-on: https://chromium-review.googlesource.com/c/1317049 Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/master@{nodejs#57261} Refs: v8/v8@0483e9a

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.

nodejs-github-bot · 2018-11-06T11:24:53Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1510/pipeline

joyeecheung · 2018-11-06T11:26:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18363/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/259/

cc @addaleax @hashseed

joyeecheung · 2018-11-06T12:54:26Z

From the Benchmark CI:

Benchmark results

                                                                        confidence improvement accuracy (*)   (**)   (***)
 fs/bench-readdir.js withFileTypes='false' dir='lib' n=10                       **      4.16 %       ±2.84% ±3.78%  ±4.92%
 fs/bench-readdir.js withFileTypes='false' dir='test/parallel' n=10            ***     16.63 %       ±2.50% ±3.32%  ±4.33%
 fs/bench-readdir.js withFileTypes='true' dir='lib' n=10                        **      4.58 %       ±3.15% ±4.20%  ±5.49%
 fs/bench-readdir.js withFileTypes='true' dir='test/parallel' n=10             ***     12.46 %       ±2.22% ±2.96%  ±3.86%
 fs/bench-readdirSync.js withFileTypes='false' dir='lib' n=10                           6.01 %       ±7.39% ±9.84% ±12.81%
 fs/bench-readdirSync.js withFileTypes='false' dir='test/parallel' n=10        ***     18.95 %       ±5.27% ±7.02%  ±9.17%
 fs/bench-readdirSync.js withFileTypes='true' dir='lib' n=10                     *      5.94 %       ±5.64% ±7.51%  ±9.77%
 fs/bench-readdirSync.js withFileTypes='true' dir='test/parallel' n=10         ***     14.33 %       ±4.42% ±5.89%  ±7.68%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 8 comparisons, you can thus
expect the following amount of false-positive results:
  0.40 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.08 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)
Notifying upstream projects of job completion
Finished: SUCCESS

Significant impact

                                                                        confidence improvement accuracy (*)   (**)   (***)
 fs/bench-readdir.js withFileTypes='false' dir='lib' n=10                       **      4.16 %       ±2.84% ±3.78%  ±4.92%
 fs/bench-readdir.js withFileTypes='false' dir='test/parallel' n=10            ***     16.63 %       ±2.50% ±3.32%  ±4.33%
 fs/bench-readdir.js withFileTypes='true' dir='lib' n=10                        **      4.58 %       ±3.15% ±4.20%  ±5.49%
 fs/bench-readdir.js withFileTypes='true' dir='test/parallel' n=10             ***     12.46 %       ±2.22% ±2.96%  ±3.86%
 fs/bench-readdirSync.js withFileTypes='false' dir='test/parallel' n=10        ***     18.95 %       ±5.27% ±7.02%  ±9.17%
 fs/bench-readdirSync.js withFileTypes='true' dir='lib' n=10                     *      5.94 %       ±5.64% ±7.51%  ±9.77%
 fs/bench-readdirSync.js withFileTypes='true' dir='test/parallel' n=10         ***     14.33 %       ±4.42% ±5.89%  ±7.68%

bnoordhuis

LGTM with suggestions. Nice work.

bnoordhuis · 2018-11-06T18:32:59Z

benchmark/fs/bench-readdir.js

    if (cntr-- <= 0)
      return bench.end(n);
-    fs.readdir(path.resolve(__dirname, '../../lib/'), function() {
+    const fullPath = path.resolve(__dirname, '../../', dir);


Maybe something for a separate commit or PR but you could move this to before the bench.start() call. The fewer tangents the benchmark has, the better.

(Ditto in the other file.)

Done, thanks for spotting this! (Pretty sure I made a change for that before, somehow it got lost before I opened the PR..)

bnoordhuis · 2018-11-06T18:35:52Z

src/node_file.cc

-  result->Set(0, names);
-  result->Set(1, types);
+  result->Set(0, Array::New(isolate, name_v.data(), name_v.size()));
+  result->Set(1, Array::New(isolate, type_v.data(), type_v.size()));


You could improve on this a little by returning a single names+types array. You also need to update getDirents() in lib/internal/fs/utils.js if you do, but that should be straightforward.

@bnoordhuis Good idea, though I'd rather leave that as a follow-up

joyeecheung · 2018-11-06T20:51:57Z

(The CI seems silent, so hopefully I am not getting in the way of code and learn): I don't think I could break anything with the fixup but anyway another CI https://ci.nodejs.org/job/node-test-pull-request/18377/

refack

🎉

Original commit message: [api] Allow embedder to construct an Array from Local<Value>* Currently to obtain a v8::Array out of a C array or a std::vector, one needs to loop through the elements and call array->Set() multiple times, and these calls go into v8::Object::Set() which can be slow. This patch adds a new Array::New overload that converts a Local<Value>* with known size into a Local<Array>. Change-Id: I0a768f0e18eec51e78d58be455482ec6425ca188 Reviewed-on: https://chromium-review.googlesource.com/c/1317049 Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/master@{nodejs#57261} Refs: v8/v8@0483e9a PR-URL: nodejs#24125 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

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#24125 Refs: v8/v8@0483e9a Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: nodejs#24125 Refs: v8/v8@0483e9a Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Trott · 2018-11-08T23:04:37Z

Landed in 7d86a53...ca51b94

Original commit message: [api] Allow embedder to construct an Array from Local<Value>* Currently to obtain a v8::Array out of a C array or a std::vector, one needs to loop through the elements and call array->Set() multiple times, and these calls go into v8::Object::Set() which can be slow. This patch adds a new Array::New overload that converts a Local<Value>* with known size into a Local<Array>. Change-Id: I0a768f0e18eec51e78d58be455482ec6425ca188 Reviewed-on: https://chromium-review.googlesource.com/c/1317049 Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/master@{#57261} Refs: v8/v8@0483e9a PR-URL: #24125 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

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: #24125 Refs: v8/v8@0483e9a Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #24125 Refs: v8/v8@0483e9a Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Original commit message: [api] Allow embedder to construct an Array from Local<Value>* Currently to obtain a v8::Array out of a C array or a std::vector, one needs to loop through the elements and call array->Set() multiple times, and these calls go into v8::Object::Set() which can be slow. This patch adds a new Array::New overload that converts a Local<Value>* with known size into a Local<Array>. Change-Id: I0a768f0e18eec51e78d58be455482ec6425ca188 Reviewed-on: https://chromium-review.googlesource.com/c/1317049 Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/master@{nodejs#57261} Refs: v8/v8@0483e9a PR-URL: nodejs#24125 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

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#24125 Refs: v8/v8@0483e9a Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: nodejs#24125 Refs: v8/v8@0483e9a Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

replace with C++ API. Refs: nodejs#24125

replace with C++ API. Refs: #24125 PR-URL: #24614 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

Original commit message: [api] Allow embedder to construct an Array from Local<Value>* Currently to obtain a v8::Array out of a C array or a std::vector, one needs to loop through the elements and call array->Set() multiple times, and these calls go into v8::Object::Set() which can be slow. This patch adds a new Array::New overload that converts a Local<Value>* with known size into a Local<Array>. Change-Id: I0a768f0e18eec51e78d58be455482ec6425ca188 Reviewed-on: https://chromium-review.googlesource.com/c/1317049 Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/master@{nodejs#57261} Refs: v8/v8@0483e9a PR-URL: nodejs#24125 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

codebytere · 2018-12-13T22:06:54Z

@joyeecheung is this a candidate for v10.x? if so, could you please backport it?

replace with C++ API. Refs: nodejs#24125 PR-URL: nodejs#24614 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

Original commit message: [api] Allow embedder to construct an Array from Local<Value>* Currently to obtain a v8::Array out of a C array or a std::vector, one needs to loop through the elements and call array->Set() multiple times, and these calls go into v8::Object::Set() which can be slow. This patch adds a new Array::New overload that converts a Local<Value>* with known size into a Local<Array>. Change-Id: I0a768f0e18eec51e78d58be455482ec6425ca188 Reviewed-on: https://chromium-review.googlesource.com/c/1317049 Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/master@{nodejs#57261} Refs: v8/v8@0483e9a PR-URL: nodejs#24125 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yang Guo <yangguo@chromium.org> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

joyeecheung added 3 commits November 6, 2018 18:05

fs: replace pushValueToArray with pure C++ API

e02f5a6

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.

benchmark: add dir and withFileTypes option readdir benchmarks

72c6b4b

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 6, 2018

addaleax approved these changes Nov 6, 2018

View reviewed changes

hashseed approved these changes Nov 6, 2018

View reviewed changes

devsnek approved these changes Nov 6, 2018

View reviewed changes

cjihrig approved these changes Nov 6, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 6, 2018

bnoordhuis approved these changes Nov 6, 2018

View reviewed changes

fixup! benchmark: add dir and withFileTypes option readdir benchmarks

beb263d

refack approved these changes Nov 6, 2018

View reviewed changes

Trott closed this Nov 8, 2018

refack added landed and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 8, 2018

joyeecheung mentioned this pull request Nov 9, 2018

src: remove pushValueToArray and setupProcessObject #24264

Closed

3 tasks

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

hiroppy mentioned this pull request Nov 24, 2018

Code And Learn 2018 nodejsjp/node-1#1

Closed

62 tasks

kt3k mentioned this pull request Nov 24, 2018

src: migrate to new V8 array API #24613

Closed

2 tasks

kazupon added a commit to kazupon/node that referenced this pull request Nov 24, 2018

src: replace array implementation

e7c4aa5

replace with C++ API. Refs: nodejs#24125

kazupon mentioned this pull request Nov 24, 2018

src: replace array implementation #24614

Closed

4 tasks

codebytere added the backport-requested-v10.x label Dec 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: replace pushValueToArray with pure C++ API#24125

src: replace pushValueToArray with pure C++ API#24125
joyeecheung wants to merge 4 commits intonodejs:masterfrom
joyeecheung:readdir-benchmark

joyeecheung commented Nov 6, 2018 •

edited

Loading

Uh oh!

nodejs-github-bot commented Nov 6, 2018

Uh oh!

joyeecheung commented Nov 6, 2018

Uh oh!

joyeecheung commented Nov 6, 2018 •

edited

Loading

Uh oh!

bnoordhuis left a comment

Uh oh!

bnoordhuis Nov 6, 2018

Uh oh!

joyeecheung Nov 6, 2018

Uh oh!

bnoordhuis Nov 6, 2018

Uh oh!

joyeecheung Nov 6, 2018

Uh oh!

joyeecheung commented Nov 6, 2018

Uh oh!

refack left a comment

Uh oh!

Trott commented Nov 8, 2018

Uh oh!

codebytere commented Dec 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

Conversation

joyeecheung commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

deps: cherry-pick 0483e9a from upstream V8

fs: replace pushValueToArray with pure C++ API

benchmark: add dir and withFileTypes option readdir benchmarks

Checklist

Uh oh!

nodejs-github-bot commented Nov 6, 2018

Uh oh!

joyeecheung commented Nov 6, 2018

Uh oh!

joyeecheung commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Nov 6, 2018

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 8, 2018

Uh oh!

codebytere commented Dec 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

joyeecheung commented Nov 6, 2018 •

edited

Loading

deps: cherry-pick `0483e9a` from upstream V8

joyeecheung commented Nov 6, 2018 •

edited

Loading