process: code cleanup for nextTick #28047
Conversation
This comment has been minimized.
This comment has been minimized.
apapirovski
force-pushed
the
next-tick-stuffs
branch
from
ba15880 to
e4ecfe2
Compare
Fix a few edge cases and non-obvious issues with nextTick: 1. Emit destroy hook in a try-finally rather than triggering it before the callback runs. 2. Re-word comment for processPromiseRejections and make sure it returns true in the rejectionHandled case too. 3. Small readability improvements.
apapirovski
force-pushed
the
next-tick-stuffs
branch
from
e4ecfe2 to
1a66c0d
Compare
This comment has been minimized.
This comment has been minimized.
BridgeAR
added
author ready
|
@apapirovski would it be possible to write a test case for the destroy hook change? |
| function processPromiseRejections() { | ||
| let maybeScheduledTicksOrMicrotasks = asyncHandledRejections.length > 0; |
There was a problem hiding this comment.
On a side note, I wonder if it make sense now to just merge process/promises.js and process/task_queues.js - the latter is currently the only one who requires the former.
There was a problem hiding this comment.
Maybe? At the same time it's a little easier to read the task queue code as is. Also, the code in promises isn't really explicitly tied to task queues.
If someone feels strongly, they should open a PR. I, personally, like it as is.
There was a problem hiding this comment.
Keep in mind, more files = more startup time.
There was a problem hiding this comment.
@Fishrock123 This is now somewhat negligible since we have shipped pre-built code cache for builtin modules, which eliminates the majority of the overhead of loading an builtin module (compilation) - of course that does not apply to modules like internal/errors.js which does a lot of stuff in the initialization, but that overhead would be eliminated as well when the v8 snapshot integration is completed.
|
@BridgeAR added a test, as requested. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Worth a benchmark run? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Timers benchmark with this PR: https://ci.nodejs.org/view/All/job/benchmark-node-micro-benchmarks/392/ |
Fix a few edge cases and non-obvious issues with nextTick: 1. Emit destroy hook in a try-finally rather than triggering it before the callback runs. 2. Re-word comment for processPromiseRejections and make sure it returns true in the rejectionHandled case too. 3. Small readability improvements. PR-URL: nodejs#28047 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
|
Landed in abf765e |
Fix a few edge cases and non-obvious issues with nextTick: 1. Emit destroy hook in a try-finally rather than triggering it before the callback runs. 2. Re-word comment for processPromiseRejections and make sure it returns true in the rejectionHandled case too. 3. Small readability improvements. PR-URL: #28047 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Fix a few edge cases and non-obvious issues with nextTick:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes