Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timers: recent performance regressions #41219

Closed
mscdex opened this issue Dec 17, 2021 · 3 comments · Fixed by #41231
Closed

timers: recent performance regressions #41219

mscdex opened this issue Dec 17, 2021 · 3 comments · Fixed by #41231
Labels
performance Issues and PRs related to the performance of Node.js. process Issues and PRs related to the process subsystem. regression Issues related to regressions. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@mscdex
Copy link
Contributor

mscdex commented Dec 17, 2021

0d9f3bd introduced a noticeable performance regression with various timers benchmarks. For example:

  • timers/immediate.js type="clear" performance dropped 77%
  • timers/timers-cancel-pooled.js performance dropped 55%
  • timers/timers-timeout-pooled.js performance dropped 41%

and many other timers benchmarks/benchmark configs saw double digit drops.

While having a public "get active requests"/"get active handles" API would be nice, I don't think it is worth this kind of penalty if we can't find some way to remedy the performance regressions.

/cc @RaisinTen

@mscdex mscdex added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. process Issues and PRs related to the process subsystem. performance Issues and PRs related to the performance of Node.js. regression Issues related to regressions. labels Dec 17, 2021
@RaisinTen
Copy link
Contributor

To mitigate this, we can traverse

const timerListMap = ObjectCreate(null);
for the Timeouts and
const outstandingQueue = new ImmediateList();
for the Immediates. That way, we wouldn't have to run any extra code when the timers are added/removed. The results are going to be a little less accurate because these objects aren't updated at the right moment always (which is why I didn't do it that way in #40813) but we can always fix that in a separate PR.

RaisinTen added a commit to RaisinTen/node that referenced this issue Dec 18, 2021
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 traversing the timerListMap and
outstandingQueue objects to count the active timers inside
process.getActiveResourcesInfo().

Fixes: nodejs#41219

Signed-off-by: Darshan Sen <darshan.sen@postman.com>
@RaisinTen
Copy link
Contributor

PR: #41231

@Trott
Copy link
Member

Trott commented Dec 18, 2021

we can always fix that in a separate PR

And if we can't fix it, we can always just leave it documented as an API limitation.

RaisinTen added a commit to RaisinTen/node that referenced this issue Dec 31, 2021
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>
nodejs-github-bot pushed a commit that referenced this issue Jan 2, 2022
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>
targos pushed a commit that referenced this issue Jan 14, 2022
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>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
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>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
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>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues and PRs related to the performance of Node.js. process Issues and PRs related to the process subsystem. regression Issues related to regressions. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants