Uh oh!
There was an error while loading. Please reload this page.
timers: use ref counts to count timers - #41231
Conversation
nodejs-github-bot
commented
Dec 18, 2021
Review 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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
RaisinTen
commented
Dec 23, 2021
The benchmark results look very similar to the results of the revert PR - #41298. I think we should go ahead with this change because the slowdown in |
This comment has been minimized.
This comment has been minimized.
apapirovski
left a comment
There was a problem hiding this comment.
I left some comments but honestly they're all superfluous because this and the original PR are both doing entirely too much work. Both Timer & Immediate already do their own book-keeping in JS-land for the number of active objects. You just need to export a function that returns refCount & immediateInfo[kRefCount] from internal/timers.js.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The additional objects that were getting added and deleted from the activeTimersMap object were slowing down the rest of the timers code, so this change falls back to using the ref counts to count the active timers inside process.getActiveResourcesInfo(). Fixes: nodejs#41219 Signed-off-by: Darshan Sen <darshan.sen@postman.com>
157ced8 to
4446606ComparetimerListMap and outstandingQueue to count timersRaisinTen
commented
Dec 31, 2021
@apapirovski thanks for the review! I've updated the PR to use refCount and immediateInfo[kRefCount] to count the timers, PTAL. |
This comment has been minimized.
This comment has been minimized.
Timers benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1080/ |
nodejs-github-bot
commented
Dec 31, 2021
apapirovski
left a comment
There was a problem hiding this comment.
Looks good. Thanks for making the change!
aduh95
left a comment
There was a problem hiding this comment.
Benchmark results look impressive 👏
nodejs-github-bot
commented
Jan 2, 2022
Landed in 64cc066 |
The additional objects that were getting added and deleted from the activeTimersMap object were slowing down the rest of the timers code, so this change falls back to using the ref counts to count the active timers inside process.getActiveResourcesInfo(). Fixes: #41219 Signed-off-by: Darshan Sen <darshan.sen@postman.com> PR-URL: #41231 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
The additional objects that were getting added and deleted from the activeTimersMap object were slowing down the rest of the timers code, so this change falls back to using the ref counts to count the active timers inside process.getActiveResourcesInfo(). Fixes: #41219 Signed-off-by: Darshan Sen <darshan.sen@postman.com> PR-URL: #41231 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
The additional objects that were getting added and deleted from the activeTimersMap object were slowing down the rest of the timers code, so this change falls back to using the ref counts to count the active timers inside process.getActiveResourcesInfo(). Fixes: #41219 Signed-off-by: Darshan Sen <darshan.sen@postman.com> PR-URL: #41231 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
The additional objects that were getting added and deleted from the
activeTimersMapobject were slowing down the rest of the timers code,so this change falls back to using the ref counts to count the active
timers inside
process.getActiveResourcesInfo().Fixes: #41219
Signed-off-by: Darshan Sen darshan.sen@postman.com
cc @mscdex @nodejs/timers