perf_hooks: performance milestone time origin timestamp improvement #51713

IlyasShabi · 2024-02-09T19:39:28Z

local benchmark:

                                                        confidence improvement accuracy (*)   (**)  (***)
perf_hooks/time-origin.js method='timeOrigin' n=1000000        ***     75.84 %       ±1.07% ±1.44% ±1.91%
perf_hooks/time-origin.js method='toJSON' n=1000000            ***     27.41 %       ±1.24% ±1.66% ±2.16%

H4ad

Is always nice to see more usage of Fast APIs, thanks for the PR!

joyeecheung · 2024-02-12T16:30:17Z

Not blocking but just a reminder - it would be faster if we just pass env->time_origin_timestamp() the same way we pass env->time_origin() to JS land via a typed array, and don't do a native call at all, because this is conceptually a constant (I am a bit surprised that it's not done already since we've been doing this for env->time_origin(), though with a closer look it seems to be only used by performance.nodeTiming.

nodejs-github-bot · 2024-02-17T15:35:34Z

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

IlyasShabi · 2024-02-19T11:57:53Z

Thanks for the suggestion @joyeecheung ! I've changed the code to pass env->time_origin_timestamp() the same way we do with env->time_origin(). It's now faster. Here is the benchmarks to show the improvement.

                                                        confidence improvement accuracy (*)   (**)  (***)
perf_hooks/time-origin.js method='timeOrigin' n=1000000        ***    120.57 %       ±2.46% ±3.30% ±4.35%
perf_hooks/time-origin.js method='toJSON' n=1000000            ***     32.10 %       ±1.64% ±2.18% ±2.86%

I've updated the commit message and PR title since it's no more a Fast api implementation, could you please review it again 🙏

joyeecheung

LGTM % comments. The timestamp is already a double and there is no need to cast it to uint64_t on the way.

src/node_perf.cc

src/node_perf_common.h

nodejs-github-bot · 2024-02-21T17:28:59Z

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

nodejs-github-bot · 2024-02-25T00:26:01Z

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

nodejs-github-bot · 2024-02-27T08:38:20Z

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

IlyasShabi · 2024-02-27T10:33:37Z

Could someone merge this ? 👀

nodejs-github-bot · 2024-02-27T10:43:38Z

Commit Queue failed

- Loading data for nodejs/node/pull/51713
✔  Done loading data for nodejs/node/pull/51713
----------------------------------- PR info ------------------------------------
Title      perf_hooks: performance milestone time origin timestamp improvement (#51713)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     IlyasShabi:fast-time-origin-timestamp -> nodejs:main
Labels     c++, needs-ci, needs-benchmark-ci
Commits    1
 - perf_hooks: performance milestone time origin timestamp improvement
Committers 1
 - Ilyas Shabi 
PR-URL: https://github.com/nodejs/node/pull/51713
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Marco Ippolito 
Reviewed-By: Minwoo Jung 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Joyee Cheung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51713
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Marco Ippolito 
Reviewed-By: Minwoo Jung 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Joyee Cheung 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - perf_hooks: performance milestone time origin timestamp improvement
   ℹ  This PR was created on Fri, 09 Feb 2024 19:39:28 GMT
   ✔  Approvals: 6
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/51713#pullrequestreview-1873159855
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/51713#pullrequestreview-1873194489
   ✔  - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/51713#pullrequestreview-1873580594
   ✔  - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/51713#pullrequestreview-1874371237
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/51713#pullrequestreview-1874747501
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/51713#pullrequestreview-1889138330
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-02-27T08:38:20Z: https://ci.nodejs.org/job/node-test-pull-request/57449/
- Querying data for job/node-test-pull-request/57449/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

nodejs-github-bot · 2024-02-28T16:52:31Z

Landed in f4af4b1

PR-URL: #51713 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs#51713 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 9, 2024

IlyasShabi force-pushed the fast-time-origin-timestamp branch 2 times, most recently from 5d0062f to 69e1fe6 Compare February 9, 2024 19:47

H4ad approved these changes Feb 9, 2024

View reviewed changes

anonrig approved these changes Feb 9, 2024

View reviewed changes

anonrig added the needs-benchmark-ci PR that need a benchmark CI run. label Feb 9, 2024

marco-ippolito approved these changes Feb 10, 2024

View reviewed changes

JungMinu approved these changes Feb 11, 2024

View reviewed changes

benjamingr approved these changes Feb 12, 2024

View reviewed changes

mertcanaltin approved these changes Feb 17, 2024

View reviewed changes

H4ad added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2024

IlyasShabi force-pushed the fast-time-origin-timestamp branch from 69e1fe6 to ad89f83 Compare February 19, 2024 11:56

IlyasShabi changed the title ~~perf_hooks: Implement performance.timeOrigin and performance.toJSON() with fast API calls~~ perf_hooks: performance milestone time origin timestamp improvement Feb 19, 2024

joyeecheung approved these changes Feb 19, 2024

View reviewed changes

IlyasShabi force-pushed the fast-time-origin-timestamp branch from ad89f83 to 5cd317b Compare February 19, 2024 21:01

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2024

IlyasShabi force-pushed the fast-time-origin-timestamp branch from 5cd317b to f5e6847 Compare February 24, 2024 23:47

H4ad added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2024

perf_hooks: performance milestone time origin timestamp improvement

f098676

IlyasShabi force-pushed the fast-time-origin-timestamp branch from f5e6847 to f098676 Compare February 26, 2024 15:23

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 27, 2024

joyeecheung approved these changes Feb 28, 2024

View reviewed changes

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 28, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 28, 2024

nodejs-github-bot merged commit f4af4b1 into nodejs:main Feb 28, 2024
61 checks passed

marco-ippolito mentioned this pull request Mar 1, 2024

v21.7.0 proposal #51932

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf_hooks: performance milestone time origin timestamp improvement #51713

perf_hooks: performance milestone time origin timestamp improvement #51713

IlyasShabi commented Feb 9, 2024

H4ad left a comment

joyeecheung commented Feb 12, 2024 •

edited

Loading

nodejs-github-bot commented Feb 17, 2024

IlyasShabi commented Feb 19, 2024

joyeecheung left a comment

nodejs-github-bot commented Feb 21, 2024

nodejs-github-bot commented Feb 25, 2024

nodejs-github-bot commented Feb 27, 2024

IlyasShabi commented Feb 27, 2024

nodejs-github-bot commented Feb 27, 2024

nodejs-github-bot commented Feb 28, 2024

perf_hooks: performance milestone time origin timestamp improvement #51713

perf_hooks: performance milestone time origin timestamp improvement #51713

Conversation

IlyasShabi commented Feb 9, 2024

H4ad left a comment

Choose a reason for hiding this comment

joyeecheung commented Feb 12, 2024 • edited Loading

nodejs-github-bot commented Feb 17, 2024

IlyasShabi commented Feb 19, 2024

joyeecheung left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 21, 2024

nodejs-github-bot commented Feb 25, 2024

nodejs-github-bot commented Feb 27, 2024

IlyasShabi commented Feb 27, 2024

nodejs-github-bot commented Feb 27, 2024

nodejs-github-bot commented Feb 28, 2024

joyeecheung commented Feb 12, 2024 •

edited

Loading