timers: use V8 fast API calls #46579

joyeecheung · 2023-02-09T00:16:22Z

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

nodejs-github-bot · 2023-02-09T00:16:26Z

Review requested:

@nodejs/gyp
@nodejs/startup

test/parallel/test-timers-ordering.js

jasnell · 2023-02-09T00:27:20Z

src/timers.h

+  static void SetupTimers(const v8::FunctionCallbackInfo<v8::Value>& args);
+
+  static void SlowGetLibuvNow(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static double FastGetLibuvNow(v8::Local<v8::Object> receiver);


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

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).

Maybe we can add this as an example to https://github.com/nodejs/node/blob/main/doc/contributing/adding-v8-fast-api.md?

anonrig

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

anonrig · 2023-02-09T01:14:56Z

cc @nodejs/performance

mcollina

lgtm

bnoordhuis · 2023-02-09T10:35:24Z

@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

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

lib/internal/timers.js

src/timers.h

github-actions · 2023-02-10T15:25:47Z

Failed to start CI

- Validating Jenkins credentials
✖  Jenkins credentials invalid

https://github.com/nodejs/node/actions/runs/4145265543

mcollina

lgtm

joyeecheung · 2023-02-11T23:54:26Z

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

nodejs-github-bot · 2023-02-22T11:22:05Z

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

joyeecheung · 2023-02-24T16:45:39Z

Rebased after #46556

nodejs-github-bot · 2023-02-24T16:46:11Z

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

nodejs-github-bot · 2023-02-27T13:49:44Z

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

nodejs-github-bot · 2023-02-27T17:12:46Z

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

nodejs-github-bot · 2023-02-27T22:51:17Z

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

joyeecheung · 2023-02-27T22:51:21Z

Rebased again to fix merge conflict

mcollina

lgtm

nodejs-github-bot · 2023-02-28T09:37:27Z

Landed in 1101713

PR-URL: #46579 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

danielleadams · 2023-04-03T05:06:52Z

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

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

joyeecheung force-pushed the timer-fast-call branch from a97da50 to 03ec71e Compare February 9, 2023 00:17

jasnell reviewed Feb 9, 2023

View reviewed changes

test/parallel/test-timers-ordering.js Show resolved Hide resolved

jasnell reviewed Feb 9, 2023

View reviewed changes

jasnell approved these changes Feb 9, 2023

View reviewed changes

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

anonrig approved these changes Feb 9, 2023

View reviewed changes

ronag approved these changes Feb 9, 2023

View reviewed changes

mcollina approved these changes Feb 9, 2023

View reviewed changes

RaisinTen approved these changes Feb 9, 2023

View reviewed changes

cjihrig approved these changes Feb 9, 2023

View reviewed changes

tniessen approved these changes Feb 9, 2023

View reviewed changes

benjamingr approved these changes Feb 9, 2023

View reviewed changes

VoltrexKeyva reviewed Feb 9, 2023

View reviewed changes

lib/internal/timers.js Outdated Show resolved Hide resolved

src/timers.h Outdated Show resolved Hide resolved

src/timers.h Outdated Show resolved Hide resolved

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

mcollina approved these changes Feb 10, 2023

View reviewed changes

joyeecheung force-pushed the timer-fast-call branch from 6621a78 to 9567958 Compare February 11, 2023 23:55

joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Feb 11, 2023

joyeecheung force-pushed the timer-fast-call branch from 9567958 to f1c67f6 Compare February 12, 2023 00:06

joyeecheung mentioned this pull request Feb 13, 2023

src: use an array for faster binding data lookup #46620

Closed

ronag mentioned this pull request Feb 20, 2023

getLibuvNow is slow nodejs/performance#49

Closed

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

This was referenced Feb 23, 2023

CI Reliability 2023-02-23 nodejs/reliability#504

Open

CI Reliability 2023-02-24 nodejs/reliability#505

Open

joyeecheung force-pushed the timer-fast-call branch from f1c67f6 to 1c1339a Compare February 24, 2023 16:45

timers: use V8 fast API calls

ca852e3

joyeecheung force-pushed the timer-fast-call branch from 1c1339a to ca852e3 Compare February 27, 2023 22:49

github-actions bot mentioned this pull request Feb 28, 2023

CI Reliability 2023-02-28 nodejs/reliability#509

Open

16 tasks

mcollina approved these changes Feb 28, 2023

View reviewed changes

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

github-actions bot mentioned this pull request Mar 1, 2023

CI Reliability 2023-03-01 nodejs/reliability#510

Open

15 tasks

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Apr 3, 2023

KhafraDev mentioned this pull request Sep 5, 2024

fix: use fasttimers for all connection timeouts nodejs/undici#3552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

timers: use V8 fast API calls #46579

timers: use V8 fast API calls #46579

joyeecheung commented Feb 9, 2023 •

edited by anonrig

Loading

nodejs-github-bot commented Feb 9, 2023

jasnell Feb 9, 2023

joyeecheung Feb 9, 2023

anonrig Feb 9, 2023

anonrig left a comment

anonrig commented Feb 9, 2023

mcollina left a comment

bnoordhuis commented Feb 9, 2023

benjamingr left a comment

github-actions bot commented Feb 10, 2023

mcollina left a comment

joyeecheung commented Feb 11, 2023

nodejs-github-bot commented Feb 22, 2023

joyeecheung commented Feb 24, 2023

nodejs-github-bot commented Feb 24, 2023

nodejs-github-bot commented Feb 27, 2023

nodejs-github-bot commented Feb 27, 2023

nodejs-github-bot commented Feb 27, 2023

joyeecheung commented Feb 27, 2023

mcollina left a comment

nodejs-github-bot commented Feb 28, 2023

danielleadams commented Apr 3, 2023

timers: use V8 fast API calls #46579

timers: use V8 fast API calls #46579

Conversation

joyeecheung commented Feb 9, 2023 • edited by anonrig Loading

nodejs-github-bot commented Feb 9, 2023

jasnell Feb 9, 2023

Choose a reason for hiding this comment

joyeecheung Feb 9, 2023

Choose a reason for hiding this comment

anonrig Feb 9, 2023

Choose a reason for hiding this comment

anonrig left a comment

Choose a reason for hiding this comment

anonrig commented Feb 9, 2023

mcollina left a comment

Choose a reason for hiding this comment

bnoordhuis commented Feb 9, 2023

benjamingr left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 10, 2023

mcollina left a comment

Choose a reason for hiding this comment

joyeecheung commented Feb 11, 2023

nodejs-github-bot commented Feb 22, 2023

joyeecheung commented Feb 24, 2023

nodejs-github-bot commented Feb 24, 2023

nodejs-github-bot commented Feb 27, 2023

nodejs-github-bot commented Feb 27, 2023

nodejs-github-bot commented Feb 27, 2023

joyeecheung commented Feb 27, 2023

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 28, 2023

danielleadams commented Apr 3, 2023

joyeecheung commented Feb 9, 2023 •

edited by anonrig

Loading