timers: use ref counts to count timers #41231

RaisinTen · 2021-12-18T12:24:32Z

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

cc @mscdex @nodejs/timers

nodejs-github-bot · 2021-12-18T12:24:36Z

Review requested:

@nodejs/startup

lib/internal/bootstrap/node.js

RaisinTen · 2021-12-23T11:15:53Z

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 timers/timers-breadth-args.js and timers/timers-insert-pooled.js is present in the revert PR too.

apapirovski

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.

lib/internal/bootstrap/node.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>

RaisinTen · 2021-12-31T09:43:55Z

@apapirovski thanks for the review! I've updated the PR to use refCount and immediateInfo[kRefCount] to count the timers, PTAL.

RaisinTen · 2021-12-31T09:47:56Z

Timers benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1080/

                                                             confidence improvement accuracy (*)    (**)   (***)
timers/immediate.js type='breadth1' n=5000000                               -1.17 %       ±4.24%  ±5.65%  ±7.38%
timers/immediate.js type='breadth4' n=5000000                        **      6.94 %       ±4.99%  ±6.67%  ±8.76%
timers/immediate.js type='breadth' n=5000000                        ***     19.86 %       ±5.15%  ±6.88%  ±8.98%
timers/immediate.js type='clear' n=5000000                          ***    282.93 %       ±4.54%  ±6.11%  ±8.11%
timers/immediate.js type='depth1' n=5000000                         ***     41.79 %       ±1.93%  ±2.58%  ±3.38%
timers/immediate.js type='depth' n=5000000                          ***     41.20 %       ±1.26%  ±1.67%  ±2.17%
timers/set-immediate-breadth-args.js n=5000000                              -0.07 %       ±4.01%  ±5.35%  ±6.99%
timers/set-immediate-breadth.js n=10000000                                  -1.82 %       ±3.54%  ±4.71%  ±6.15%
timers/set-immediate-depth-args.js n=5000000                        ***     39.03 %       ±2.49%  ±3.31%  ±4.31%
timers/timers-breadth-args.js n=1000000                             ***    -12.75 %       ±5.98%  ±8.00% ±10.51%
timers/timers-breadth.js n=5000000                                           2.47 %       ±3.74%  ±4.98%  ±6.50%
timers/timers-cancel-pooled.js n=5000000                            ***    188.38 %      ±24.54% ±32.95% ±43.50%
timers/timers-cancel-unpooled.js direction='end' n=1000000           **     24.24 %      ±15.35% ±20.48% ±26.75%
timers/timers-cancel-unpooled.js direction='start' n=1000000          *      9.86 %       ±8.87% ±11.81% ±15.39%
timers/timers-depth.js n=1000                                               -0.24 %       ±0.47%  ±0.63%  ±0.82%
timers/timers-insert-pooled.js n=5000000                            ***    -17.76 %       ±3.28%  ±4.36%  ±5.68%
timers/timers-insert-unpooled.js direction='end' n=1000000          ***     11.50 %       ±4.24%  ±5.65%  ±7.36%
timers/timers-insert-unpooled.js direction='start' n=1000000        ***     10.90 %       ±3.99%  ±5.31%  ±6.91%
timers/timers-timeout-nexttick.js n=50000                           ***     18.12 %       ±6.26%  ±8.35% ±10.93%
timers/timers-timeout-nexttick.js n=5000000                         ***     70.62 %       ±8.74% ±11.63% ±15.16%
timers/timers-timeout-pooled.js n=10000000                          ***     93.09 %      ±11.97% ±16.06% ±21.16%
timers/timers-timeout-unpooled.js n=1000000                         ***     88.09 %      ±14.55% ±19.37% ±25.22%

nodejs-github-bot · 2021-12-31T12:26:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/41707/

apapirovski

Looks good. Thanks for making the change!

aduh95

Benchmark results look impressive 👏

nodejs-github-bot · 2022-01-02T15:28:24Z

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: 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>

nodejs-github-bot added needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Dec 18, 2021

RaisinTen mentioned this pull request Dec 18, 2021

timers: recent performance regressions #41219

Closed

This comment has been minimized.

Sign in to view

Trott requested a review from mscdex December 18, 2021 14:19

This comment has been minimized.

Sign in to view

mscdex reviewed Dec 18, 2021

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

mscdex reviewed Dec 18, 2021

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

mscdex reviewed Dec 18, 2021

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

mscdex reviewed Dec 18, 2021

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

aduh95 reviewed Dec 18, 2021

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Dec 19, 2021

CI Reliability 2021-12-19 nodejs/reliability#149

Open

24 tasks

This comment has been minimized.

Sign in to view

This was referenced Dec 20, 2021

CI Reliability 2021-12-20 nodejs/reliability#150

Open

CI Reliability 2021-12-21 nodejs/reliability#151

Open

This comment has been minimized.

Sign in to view

This was referenced Dec 22, 2021

CI Reliability 2021-12-22 nodejs/reliability#152

Open

CI Reliability 2021-12-23 nodejs/reliability#153

Open

RaisinTen mentioned this pull request Dec 23, 2021

Revert "process: add getActiveResourcesInfo()" #41298

Closed

This comment has been minimized.

Sign in to view

github-actions bot mentioned this pull request Dec 24, 2021

CI Reliability 2021-12-24 nodejs/reliability#154

Open

18 tasks

apapirovski requested changes Dec 29, 2021

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

RaisinTen force-pushed the timers/use-timerListMap-and-outstandingQueue-to-count-timers branch from 157ced8 to 4446606 Compare December 31, 2021 09:41

RaisinTen changed the title ~~timers: use timerListMap and outstandingQueue to count timers~~ timers: use ref counts to count timers Dec 31, 2021

RaisinTen requested a review from apapirovski December 31, 2021 09:44

This comment has been minimized.

Sign in to view

apapirovski approved these changes Dec 31, 2021

View reviewed changes

github-actions bot mentioned this pull request Jan 1, 2022

CI Reliability 2022-01-01 nodejs/reliability#162

Open

24 tasks

RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jan 1, 2022

aduh95 approved these changes Jan 1, 2022

View reviewed changes

github-actions bot mentioned this pull request Jan 2, 2022

CI Reliability 2022-01-02 nodejs/reliability#163

Open

23 tasks

RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 2, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 2, 2022

nodejs-github-bot merged commit 64cc066 into nodejs:master Jan 2, 2022

RaisinTen deleted the timers/use-timerListMap-and-outstandingQueue-to-count-timers branch January 2, 2022 15:28

targos mentioned this pull request Jan 16, 2022

v17.4.0 release proposal #41557

Merged

danielleadams mentioned this pull request Feb 1, 2022

v16.14.0 proposal #41804

Merged

fb55 mentioned this pull request Apr 22, 2022

Update parse5 to 7.0.0 jsdom/jsdom#3354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: use ref counts to count timers #41231

timers: use ref counts to count timers #41231

RaisinTen commented Dec 18, 2021 •

edited

nodejs-github-bot commented Dec 18, 2021

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.

RaisinTen commented Dec 23, 2021

This comment has been minimized.

apapirovski left a comment

RaisinTen commented Dec 31, 2021

This comment has been minimized.

RaisinTen commented Dec 31, 2021 •

edited

nodejs-github-bot commented Dec 31, 2021

apapirovski left a comment

aduh95 left a comment

nodejs-github-bot commented Jan 2, 2022

timers: use ref counts to count timers #41231

timers: use ref counts to count timers #41231

Conversation

RaisinTen commented Dec 18, 2021 • edited

nodejs-github-bot commented Dec 18, 2021

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.

RaisinTen commented Dec 23, 2021

This comment has been minimized.

apapirovski left a comment

Choose a reason for hiding this comment

RaisinTen commented Dec 31, 2021

This comment has been minimized.

RaisinTen commented Dec 31, 2021 • edited

nodejs-github-bot commented Dec 31, 2021

apapirovski left a comment

Choose a reason for hiding this comment

aduh95 left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jan 2, 2022

RaisinTen commented Dec 18, 2021 •

edited

RaisinTen commented Dec 31, 2021 •

edited