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: refactor to use optional chaining #36767

Closed

Conversation

Lxxyx
Copy link
Member

@Lxxyx Lxxyx commented Jan 4, 2021

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jan 4, 2021
@aduh95 aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label Jan 4, 2021
@aduh95
Copy link
Contributor

aduh95 commented Jan 5, 2021

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/874/console

No regressions or improvements
                                                              confidence improvement accuracy (*)    (**)   (***)
 timers/timers-cancel-pooled.js n=5000000                                    -0.96 %       ±9.53% ±12.68% ±16.51%
 timers/timers-cancel-unpooled.js direction='end' n=1000000                   4.08 %       ±7.54% ±10.04% ±13.07%
 timers/timers-cancel-unpooled.js direction='start' n=1000000                -2.81 %       ±5.86%  ±7.80% ±10.16%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 3 comparisons, you can thus
expect the following amount of false-positive results:
  0.15 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.03 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed needs-benchmark-ci PR that need a benchmark CI run. labels Jan 5, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Lxxyx Lxxyx force-pushed the timers-refactor-use-optional-chaining branch from fbf05e0 to 738759a Compare January 8, 2021 11:33
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

jasnell pushed a commit that referenced this pull request Jan 9, 2021
PR-URL: #36767
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 9, 2021

Landed in d8f535b

@jasnell jasnell closed this Jan 9, 2021
@Lxxyx Lxxyx deleted the timers-refactor-use-optional-chaining branch January 11, 2021 10:43
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
PR-URL: #36767
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
@mcollina
Copy link
Member

As part of #37937, I tracked down a regression introduced by this one.

With optional chaining:

Screenshot 2021-04-15 at 12 07 40

Without optional chaining:

Screenshot 2021-04-15 at 12 07 51

targos pushed a commit that referenced this pull request May 25, 2021
PR-URL: #36767
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants