node: improve GetActiveRequests performance #3375

trevnorris · 2015-10-14T21:52:09Z

v8 is faster at setting object properties in JS than C++. Even when it
requires calling into JS from native code. Make
process._getActiveRequests() faster by doing this when populating the
array containing request objects.

Simple benchmark:

for (let i = 0; i < 22; i++)
  fs.open(__filename, 'r', function() { });
let t = process.hrtime();
for (let i = 0; i < 1e6; i++)
  process._getActiveRequests();
t = process.hrtime(t);
console.log((t[0] * 1e9 + t[1]) / 1e6);

Results between the two:

Previous:  4406 ns/op
Patched:    829 ns/op     4.3x faster

R=@bnoordhuis

Have another addition of improving the same for active handles, but wanted to solicit feedback on the approach early.

rvagg · 2015-10-15T00:05:43Z

Is "index" the right word to use here? you're just appending and not actually using an index

ronkorving · 2015-10-15T01:58:07Z

While performance improvements are nice, this definitely adds more code than it removes. Is the use case here continuous monitoring? This is still an undocumented API... for what reason I don't know though, seems it would help to make it public (I've used it too, and find it very useful, but that's usually been limited to shutdown-time).

trevnorris · 2015-10-15T15:33:24Z

@rvagg That's an artifact of an early implementation. I'll find a better name.

@ronkorving The use case is to not have a crappy implementation. This is a technique that I plan to expand through core. Search for Object::Set() and you'll see how heavily we rely on this slow operation. Here was simply the easiest location to kick off.

LOC should have little to no affect on a PR. Added complexity I can understand, and can agree with based on circumstance. This though is fairly straightforward.

ronkorving · 2015-10-16T03:07:15Z

Is there any way we could help the V8 team (I say we.. as if I really could) to make Object::Set as fast as JIT compiled JavaScript?

bnoordhuis · 2015-10-17T15:54:17Z

src/node.cc

+
+  for (auto w : *env->req_wrap_queue()) {
+    if (w->persistent().IsEmpty() == false) {
+      argv[i++ % 5] = w->object();


Can you use ARRAY_SIZE(argv) instead of hard-coding it in several places? If it gets unwieldy, I suggest writing it as:

static const size_t argc = 5; Local<Value> argv[argc]; // ... argv[i++ % argc] = ...;

bnoordhuis · 2015-10-17T16:01:24Z

This could be extended to numerous other places but I assume that's your plan anyway. :-)

trevnorris · 2015-10-19T20:58:31Z

@bnoordhuis Comments addressed (I think). Yes, I am planning on extending this across core but figured this would be a good low impact place to start off. :)

bnoordhuis · 2015-10-19T22:18:15Z

src/node.cc

+  for (auto w : *env->req_wrap_queue()) {
+    if (w->persistent().IsEmpty() == false) {
+      argv[i++ % argc] = w->object();
+      if ((i % argc) == 0) {


Shouldn't this check that i > 0? It's going to make a superfluous JS call now if I read it right.

EDIT: Never mind, didn't read it right. It's never zero.

this is the most succinct way I found, but even looking back at this over the weekend I needed to remember what the logic was doing. if you have something more readable in mind I'm open to suggestions. :)

trevnorris · 2015-10-19T22:56:54Z

@bnoordhuis made most suggested changes and added a simple test.

CI: https://ci.nodejs.org/job/node-test-pull-request/536/

bnoordhuis · 2015-10-20T22:27:47Z

src/node.cc

-      ary->Set(i++, w->object());
+  if (i % argc > 0) {
+    HandleScope scope(env->isolate());
+    fn->Call(ctx, ary, i % argc, argv).ToLocalChecked();


This is the repetition I mean. I'd do the i % argc only once and cache the result in a const size_t remainder.

ah, got it. was trying to make it work w/ the for loop above.

trevnorris · 2015-10-21T18:34:29Z

@bnoordhuis comment addressed.

jasnell · 2015-10-21T18:48:25Z

Nice change, although it makes me sad that Object::Set is so slow. LGTM so long as CI is green and setPropByIndex is refactored to get away from the val0, ... val7 pattern.

v8 is faster at setting object properties in JS than C++. Even when it requires calling into JS from native code. Make process._getActiveRequests() faster by doing this when populating the array containing request objects. Simple benchmark: for (let i = 0; i < 22; i++) fs.open(__filename, 'r', function() { }); let t = process.hrtime(); for (let i = 0; i < 1e6; i++) process._getActiveRequests(); t = process.hrtime(t); console.log((t[0] * 1e9 + t[1]) / 1e6); Results between the two: Previous: 4406 ns/op Patched: 690 ns/op 5.4x faster

trevnorris · 2015-10-21T21:57:44Z

@bnoordhuis @jasnell arguments change made.

CI: https://ci.nodejs.org/job/node-test-pull-request/557/

jasnell · 2015-10-21T22:06:42Z

LGTM if CI is green

bnoordhuis · 2015-10-21T22:19:21Z

LGTM

v8 is faster at setting object properties in JS than C++. Even when it requires calling into JS from native code. Make process._getActiveRequests() faster by doing this when populating the array containing request objects. Simple benchmark: for (let i = 0; i < 22; i++) fs.open(__filename, 'r', function() { }); let t = process.hrtime(); for (let i = 0; i < 1e6; i++) process._getActiveRequests(); t = process.hrtime(t); console.log((t[0] * 1e9 + t[1]) / 1e6); Results between the two: Previous: 4406 ns/op Patched: 690 ns/op 5.4x faster PR-URL: #3375 Reviewed-By: James Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

trevnorris · 2015-10-21T22:58:36Z

None of the failures are related to the PR. Landed on 494227b. Thanks much!

v8 is faster at setting object properties in JS than C++. Even when it requires calling into JS from native code. Make process._getActiveRequests() faster by doing this when populating the array containing request objects. Simple benchmark: for (let i = 0; i < 22; i++) fs.open(__filename, 'r', function() { }); let t = process.hrtime(); for (let i = 0; i < 1e6; i++) process._getActiveRequests(); t = process.hrtime(t); console.log((t[0] * 1e9 + t[1]) / 1e6); Results between the two: Previous: 4406 ns/op Patched: 690 ns/op 5.4x faster PR-URL: #3375 Reviewed-By: James Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2016-03-11T01:58:42Z

@trevnorris what are your thought regarding adding this commit to LTS?

MylesBorins · 2016-04-11T17:11:58Z

@trevnorris ping

trevnorris · 2016-04-11T22:56:44Z

@thealphanerd oop. sorry. It's a micro-performance optimization. Merging will doubtfully prevent future conflicts. So, can land but don't think it's necessary.

trevnorris added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 14, 2015

bnoordhuis reviewed Oct 17, 2015
View reviewed changes

Fishrock123 mentioned this pull request Oct 19, 2015

v8: let Object.defineProperty work with process.env #2999

Closed

trevnorris force-pushed the make-get-faster branch from 1d1139e to 1e42a7f Compare October 19, 2015 20:57

bnoordhuis reviewed Oct 19, 2015
View reviewed changes

trevnorris force-pushed the make-get-faster branch from 1e42a7f to 2c6de7a Compare October 19, 2015 22:55

bnoordhuis reviewed Oct 20, 2015
View reviewed changes

trevnorris force-pushed the make-get-faster branch from 2c6de7a to 7daff51 Compare October 21, 2015 18:34

trevnorris force-pushed the make-get-faster branch from 7daff51 to d1dd435 Compare October 21, 2015 21:56

trevnorris closed this Oct 21, 2015

trevnorris deleted the make-get-faster branch October 23, 2015 21:48

This was referenced Oct 27, 2015

Release proposal: v5.0.0 #3466

Merged

Preparing v4.2.2 LTS Maintenance nodejs/Release#50

Closed

trevnorris mentioned this pull request Nov 11, 2015

Drop Usage of Object::Set() #3780

Closed

rvagg mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

MylesBorins mentioned this pull request Jan 14, 2016

Audit commits not found on v4.x-staging #4698

Closed

MylesBorins mentioned this pull request Mar 10, 2016

Audit commits not found on v4.x #5647

Closed

MylesBorins added the dont-land-on-v4.x label Jun 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: improve GetActiveRequests performance #3375

node: improve GetActiveRequests performance #3375

trevnorris commented Oct 14, 2015

rvagg commented Oct 15, 2015

ronkorving commented Oct 15, 2015

trevnorris commented Oct 15, 2015

ronkorving commented Oct 16, 2015

bnoordhuis Oct 17, 2015

bnoordhuis commented Oct 17, 2015

trevnorris commented Oct 19, 2015

bnoordhuis Oct 19, 2015

trevnorris Oct 19, 2015

trevnorris commented Oct 19, 2015

bnoordhuis Oct 20, 2015

trevnorris Oct 21, 2015

trevnorris commented Oct 21, 2015

jasnell commented Oct 21, 2015

trevnorris commented Oct 21, 2015

jasnell commented Oct 21, 2015

bnoordhuis commented Oct 21, 2015

trevnorris commented Oct 21, 2015

MylesBorins commented Mar 11, 2016

MylesBorins commented Apr 11, 2016

trevnorris commented Apr 11, 2016

node: improve GetActiveRequests performance #3375

node: improve GetActiveRequests performance #3375

Conversation

trevnorris commented Oct 14, 2015

rvagg commented Oct 15, 2015

ronkorving commented Oct 15, 2015

trevnorris commented Oct 15, 2015

ronkorving commented Oct 16, 2015

bnoordhuis Oct 17, 2015

Choose a reason for hiding this comment

bnoordhuis commented Oct 17, 2015

trevnorris commented Oct 19, 2015

bnoordhuis Oct 19, 2015

Choose a reason for hiding this comment

trevnorris Oct 19, 2015

Choose a reason for hiding this comment

trevnorris commented Oct 19, 2015

bnoordhuis Oct 20, 2015

Choose a reason for hiding this comment

trevnorris Oct 21, 2015

Choose a reason for hiding this comment

trevnorris commented Oct 21, 2015

jasnell commented Oct 21, 2015

trevnorris commented Oct 21, 2015

jasnell commented Oct 21, 2015

bnoordhuis commented Oct 21, 2015

trevnorris commented Oct 21, 2015

MylesBorins commented Mar 11, 2016

MylesBorins commented Apr 11, 2016

trevnorris commented Apr 11, 2016