FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

timers: use V8 fast API calls by joyeecheung · Pull Request #46579 · nodejs/node · GitHub

/ node Public

timers: use V8 fast API calls - #46579

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
joyeecheung:timer-fast-call
Feb 28, 2023
Merged

timers: use V8 fast API calls#46579
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
joyeecheung:timer-fast-call

Conversation

joyeecheung commented Feb 9, 2023
edited by anonrig
Loading

Copy link
Copy Markdown
Member

From a local run (just the timers, the immediate ones just take too long to complete...)

                                                             confidence improvement accuracy (*)   (**)  (***)
timers/timers-breadth-args.js n=1000000                             ***      5.48 %       ±2.53% ±3.38% ±4.42%
timers/timers-breadth.js n=5000000                                           3.88 %       ±4.16% ±5.53% ±7.21%
timers/timers-cancel-pooled.js n=5000000                                    -0.41 %       ±2.21% ±2.95% ±3.85%
timers/timers-cancel-unpooled.js direction='end' n=1000000                  -1.77 %       ±2.14% ±2.85% ±3.71%
timers/timers-cancel-unpooled.js direction='start' n=1000000                -0.35 %       ±1.34% ±1.79% ±2.32%
timers/timers-depth.js n=1000                                                0.46 %       ±0.82% ±1.10% ±1.43%
timers/timers-insert-pooled.js n=5000000                            ***      7.78 %       ±3.75% ±5.00% ±6.52%
timers/timers-insert-unpooled.js direction='end' n=1000000                   0.36 %       ±4.16% ±5.55% ±7.27%
timers/timers-insert-unpooled.js direction='start' n=1000000         **      4.92 %       ±3.29% ±4.39% ±5.74%
timers/timers-timeout-nexttick.js n=50000                                   -1.07 %       ±3.39% ±4.51% ±5.88%
timers/timers-timeout-nexttick.js n=5000000                           *      2.43 %       ±2.39% ±3.18% ±4.15%
timers/timers-timeout-pooled.js n=10000000                                   0.90 %       ±1.90% ±2.52% ±3.29%
timers/timers-timeout-unpooled.js n=1000000                                 -1.48 %       ±3.44% ±4.59% ±5.98%

Fixes nodejs/performance#49

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 9, 2023
Comment thread test/parallel/test-timers-ordering.js Outdated
Comment thread src/timers.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just noting: this pattern could likely use some documentation around it as we use it more.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah I thought about it and then I see that other fast API calls are doing things...slightly differently. So we probably should unify them first, or find a best pattern first? (I am not even sure this is the best pattern but for the purpose of this binding this looks the most sensible to me).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

anonrig added the performance Issues and PRs related to the performance of Node.js. label Feb 9, 2023

anonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You beat me to it! Perfect! I added the reference of the performance issue to the description.

anonrig commented Feb 9, 2023

Copy link
Copy Markdown
Member

cc @nodejs/performance

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

Copy link
Copy Markdown
Member

@joyeecheung not for this PR but if you're up for a challenge: on linux you could read the timestamp directly from the vDSO through a typed array that points to it. That should be many times faster still than V8's fast API.

(It's something I've been meaning to do for ages but never got around to.)

@billywhizz maybe an interesting technique for just-js?

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Amazing how easy it is to get consensus when we talk about this sort of thing for so long ahead of time :)

As usual great work Joyee

Comment thread lib/internal/timers.js Outdated
Comment thread src/timers.h Outdated
Comment thread src/timers.h Outdated
ronag added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Feb 10, 2023
github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Feb 10, 2023

Copy link
Copy Markdown
Contributor
Failed to start CI
- Validating Jenkins credentials
✖  Jenkins credentials invalid
https://github.com/nodejs/node/actions/runs/4145265543

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

Copy link
Copy Markdown
Member Author

Just realized that SetFastMethod() sets no side effect by default, so this needs to land on top of #46619

joyeecheung removed the blocked PRs that are blocked by other issues or PRs. label Feb 22, 2023

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Member Author

Rebased after #46556

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Member Author

Rebased again to fix merge conflict

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Feb 28, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 28, 2023
nodejs-github-bot merged commit 1101713 into nodejs:main Feb 28, 2023

Copy link
Copy Markdown
Collaborator

Landed in 1101713

targos pushed a commit that referenced this pull request Mar 13, 2023
PR-URL: #46579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
PR-URL: #46579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Copy link
Copy Markdown
Contributor

@joyeecheung this broke the v18.x build when landing. Would you be able to open a backport PR for this? Thanks!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. performance Issues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

getLibuvNow is slow

Back | FazBrowse Home | New Git URL