Skip to content

timers: recent performance regressions #41219

Closed
#41231
Closed
timers: recent performance regressions#41219
#41231
Labels
performanceIssues and PRs related to the performance of Node.js.processIssues and PRs related to the process subsystem.regressionIssues related to regressions.timersIssues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().

Description

@mscdex
mscdex
opened on 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

Activity

  1. mscdex
    added
    timersIssues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().
    processIssues and PRs related to the process subsystem.
    performanceIssues and PRs related to the performance of Node.js.
    regressionIssues related to regressions.
    on Dec 17, 2021

  2. RaisinTen commented on Dec 17, 2021

    @RaisinTen
    RaisinTen
    on Dec 17, 2021
    Member

    To mitigate this, we can traverse

    node/lib/internal/timers.js

    Line 161 in c8f5dd6

    const timerListMap = ObjectCreate(null);
    for the Timeouts and

    node/lib/internal/timers.js

    Line 426 in c8f5dd6

    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.

  3. RaisinTen commented on Dec 18, 2021

    @RaisinTen
    RaisinTen
    on Dec 18, 2021
    Member

    PR: #41231

  4. Trott commented on Dec 18, 2021

    @Trott
    Trott
    on Dec 18, 2021
    Member

    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.

  5. RaisinTen
    added a commit that references this issue on Dec 31, 2021

    timers: use ref counts to count timers

    4446606
  6. nodejs-github-bot
    closed this as completedin #41231on Jan 2, 2022
  7. nodejs-github-bot
    added a commit that references this issue on Jan 2, 2022

    timers: use ref counts to count timers

    64cc066
  8. targos
    added a commit that references this issue on Jan 14, 2022

    timers: use ref counts to count timers

    2e92f6f
  9. danielleadams
    added a commit that references this issue on Jan 31, 2022

    timers: use ref counts to count timers

    d2db729
  10. danielleadams
    added a commit that references this issue on Feb 1, 2022

    timers: use ref counts to count timers

    b5b3750
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    performanceIssues and PRs related to the performance of Node.js.processIssues and PRs related to the process subsystem.regressionIssues related to regressions.timersIssues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions