node: improve nextTick performance by BridgeAR · Pull Request #5092 · nodejs/node · GitHub

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?

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.

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

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.

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

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.

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.

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

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

LGTM

@bnoordhuis Have an opinion on the change?

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 <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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

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.

@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.

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

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 16ad4b7 Compare 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

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

node: improve nextTick performance by BridgeAR · Pull Request #5092 · nodejs/node · GitHub

Uh oh!

Uh oh!

node: improve nextTick performance - #5092

node: improve nextTick performance#5092BridgeAR wants to merge 2 commits intonodejs:masternodejs/node:masterfrom BridgeAR:masterBridgeAR/node:masterCopy head branch name to clipboard

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!

trevnorris Feb 5, 2016

Choose a reason for hiding this comment

Uh oh!

BridgeAR Feb 5, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorris Feb 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

node: improve nextTick performance#5092
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:master