node: improve nextTick performance#5092

BridgeAR · 2016-02-04T22:39:31Z

The try final block around the nextTick callback prevents optimizing the executing function. The try was introduced in a commit that was reverted later but without reverting the try code.
Initializing the array with the proper argument size is faster as using a literal.

The performance gain is between 5-10% looking at the already existing nextTick benchmarks.

@trevnorris pointed out that this is might be depending on #4507. I guess this could be handled independently though.

~~@bnoordhuis the new v8::Private api seems to change the error handling in C and after that commit the nextTick callback errors are not handled in C anymore. I'm not sure this was a intended change?~~

bnoordhuis · 2016-02-04T22:52:50Z

the new v8::Private api seems to change the error handling in C and after that commit the nextTick callback errors are not handled in C anymore. I'm not sure this was a intended change?

I'm not sure what you mean. Commit 924cc6c didn't touch error handling, only how exception objects are decorated.

BridgeAR · 2016-02-04T22:54:16Z

@bnoordhuis if you run this code without 924cc6c it'll pass. But with, it'll fail.

BridgeAR · 2016-02-04T23:01:21Z

Hm, I'm sorry I'm wrong. I'm a bit puzzled as all tests passed until I rebased to the current master but I might have only run test-parallel instead of test.

bnoordhuis · 2016-02-04T23:03:35Z

I speculate what you're seeing is the result of removing the tickDone() calls that are made when an exception is raised.

BridgeAR · 2016-02-04T23:33:54Z

Reading the error messages properly helps a lot! ;-)

While testing I did not use the _combinedTickCallback but kept the original functions in place and I did not expect any tests to fail because of that.

Prevent deoptimization of process.nextTick by removing the try finally block. This is not necessary as the next tick queue will be reset anyway, no matter if the callback throws or not.

Using a predefined array size prevents resizing the array and is therefor faster.

trevnorris · 2016-02-05T00:18:24Z

src/node.js

-nextTickCallbackWithManyArgs(callback,args);
-}
-}
+_combinedTickCallback(args,callback);


To remove a line in the call stack, think we could just move the if/else back into here? Open to why it should be kept in another function.

For me I had two reasons to split it up:
It's less duplicated code
The functions are smaller and the compiler might optimize them faster but I did not check that
I do not have any strong feelings about that though

oop. missed that it's called in multiple locations. sounds good.

trevnorris · 2016-02-05T00:36:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1555/ (on lock down ATM, will report back when complete)

mscdex · 2016-02-05T02:03:27Z

CI is all green except for a couple of flaky tests on ARM.

trevnorris · 2016-02-05T20:33:16Z

LGTM

@bnoordhuis Have an opinion on the change?

bnoordhuis · 2016-02-05T22:16:54Z

LGTM

jasnell · 2016-02-07T20:38:04Z

LGTM

Prevent deoptimization of process.nextTick by removing the try finally block. This is not necessary as the next tick queue will be reset anyway, no matter if the callback throws or not. Use a predefined array size prevents resizing the array and is therefor faster. PR-URL: #5092 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

trevnorris · 2016-02-09T09:50:56Z

Thanks much. Landed in 8830797.

Prevent deoptimization of process.nextTick by removing the try finally block. This is not necessary as the next tick queue will be reset anyway, no matter if the callback throws or not. Use a predefined array size prevents resizing the array and is therefor faster. PR-URL: #5092 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2016-02-10T04:23:58Z

Marking this for LTS but we should let it sit in master and v5 for a bit before landing

rvagg · 2016-02-10T10:24:59Z

I think I'm leaning -1 on this given that it changes the call stack. If you're relying on it for some reason (say in a debugging tool of some kind) then you'd be in trouble. I'd even be tempted to push for this as semver-major for that reason, I just doubt I'd get any other takers on that.

jasnell · 2016-02-11T16:09:12Z

@rvagg ... I'm leaning the same way. We need to let these kinds of changes sit in stable for a while before we even consider them eligible for pulling back to LTS.

trevnorris · 2016-02-11T21:20:32Z

This is a micro-performance optimization. If there's a question as to whether it should be backported, the answer is most likely no.

MylesBorins · 2016-02-18T01:45:42Z

I'm marking this as don't land for now. Please feel free to change

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354 PR-URL: #5295

Ruben Bridgewater added 2 commits February 5, 2016 00:35

node: improve process.nextTick performance
dc6403e
Prevent deoptimization of process.nextTick by removing the try finally block. This is not necessary as the next tick queue will be reset anyway, no matter if the callback throws or not.

node: improve process.nextTick performance
16ad4b7
Using a predefined array size prevents resizing the array and is therefor faster.

BridgeAR force-pushed the master branch from 2e24867 to 16ad4b7Compare February 4, 2016 23:35

trevnorris reviewed Feb 5, 2016
View reviewed changes

mscdex added the process Issues and PRs related to the process subsystem. label Feb 5, 2016

trevnorris closed this Feb 9, 2016

jasnell added the lts-watch-v4.x label Feb 10, 2016

jasnell mentioned this pull request Feb 11, 2016
refactor src/node.js into internal files #5103
Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Feb 18, 2016

rvagg mentioned this pull request Feb 18, 2016
Release proposal: 5.7.0 (Stable) #5295
Merged

ChALkeR added the performance Issues and PRs related to the performance of Node.js. label Feb 24, 2016

Uh oh!

node: improve nextTick performance#5092

node: improve nextTick performance #5092

Uh oh!

Conversation

BridgeAR commented Feb 4, 2016

Uh oh!

bnoordhuis commented Feb 4, 2016

Uh oh!

BridgeAR commented Feb 4, 2016

Uh oh!

BridgeAR commented Feb 4, 2016

Uh oh!

bnoordhuis commented Feb 4, 2016

Uh oh!

BridgeAR commented Feb 4, 2016

Uh oh!

trevnorrisFeb 5, 2016

Choose a reason for hiding this comment

Uh oh!

BridgeARFeb 5, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorrisFeb 5, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Feb 5, 2016

Uh oh!

mscdex commented Feb 5, 2016

Uh oh!

trevnorris commented Feb 5, 2016

Uh oh!

bnoordhuis commented Feb 5, 2016

Uh oh!

jasnell commented Feb 7, 2016

Uh oh!

trevnorris commented Feb 9, 2016

Uh oh!

jasnell commented Feb 10, 2016

Uh oh!

rvagg commented Feb 10, 2016

Uh oh!

jasnell commented Feb 11, 2016

Uh oh!

trevnorris commented Feb 11, 2016

Uh oh!

MylesBorins commented Feb 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants