timers: use ref counts to count timers#41231
Conversation
|
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
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.
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
4446606
Compare
timerListMap and outstandingQueue to count timers|
@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/ |
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 👏
|
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: nodejs#41219 Signed-off-by: Darshan Sen <darshan.sen@postman.com> PR-URL: nodejs#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