| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Is always nice to see more usage of Fast APIs, thanks for the PR!
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
|
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 🙏 |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM % comments. The timestamp is already a double and there is no need to cast it to uint64_t on the way.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
Could someone merge this ? 👀 |
Sorry, something went wrong.
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/.ncuhttps://github.com/nodejs/node/actions/runs/8063423832 |
Sorry, something went wrong.
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: #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>
| Back | FazBrowse Home | New Git URL |
local benchmark: