| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Just noting: this pattern could likely use some documentation around it as we use it more.
Sorry, something went wrong.
There was a problem hiding this comment.
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).
Sorry, something went wrong.
There was a problem hiding this comment.
Maybe we can add this as an example to https://github.com/nodejs/node/blob/main/doc/contributing/adding-v8-fast-api.md?
Sorry, something went wrong.
There was a problem hiding this comment.
You beat me to it! Perfect! I added the reference of the performance issue to the description.
Sorry, something went wrong.
|
cc @nodejs/performance |
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
Failed to start CI- Validating Jenkins credentials ✖ Jenkins credentials invalidhttps://github.com/nodejs/node/actions/runs/4145265543 |
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
|
Just realized that SetFastMethod() sets no side effect by default, so this needs to land on top of #46619 |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
|
Rebased again to fix merge conflict |
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
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>
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>
|
@joyeecheung this broke the v18.x build when landing. Would you be able to open a backport PR for this? Thanks! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
From a local run (just the timers, the immediate ones just take too long to complete...)
Fixes nodejs/performance#49