-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib: refactor primordials.uncurryThis #36221
Conversation
Is there a core library that's already using a lot of uncurried methods? This is to know which benchmarks to run. |
Good point, I'm not sure. Let me pull the commits from my other PRs to see if it improves the perf for buffers and fs. EDIT: Benchmark-CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/700/console |
It seems to have indeed an overall positive impact on performance. Perf regressionsWith this PR:
Without this PR:
Perf improvmentsWith this PR:
Without this PR:
|
306b14e
to
284113c
Compare
} | ||
|
||
const { bind, call } = Function.prototype; | ||
const uncurryThis = call.bind(bind, call); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not that this isn't a very cool application, but wouldn't this be a bit clearer?
const uncurryThis = (f) => Function.prototype.call.bind(f)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that Function.prototype.bind
can be modified by the user. I could add a comment explaining it though. Do you think that would be enough?
const uncurryThis = call.bind(bind, call); | |
// The following line is equivalent to `const uncurryThis = f => call.bind(f);`. | |
const uncurryThis = call.bind(bind, call); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I guess once you factor it out you just end up with call.bind(bind, call)
. A comment would be good then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, PTAL.
This is done to avoid creating an array and gain performance. Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
284113c
to
911a681
Compare
It seems to have helped a little bit, but there are still significant regressions when the other primordial changes are added on top. For example the
but with the current changes in this PR they're still:
|
@aduh95 No, I had already ran all of the Buffer benchmarks with just the (original) changes in this PR and there were no real regressions. The first set of numbers I just posted here are not including the changes in this PR and are just copied from my comment on the separate Buffer primordial changes PR and the second set is with the changes in this PR. So basically this shows that this PR does have a positive impact but it does not completely undo the regressions at least for the Buffer primordial PR. I did not benchmark the |
@mscdex OK, thanks for the clarification. So you are +1 (or +0 at least) for landing this, but your objection remains for #36166, right? |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM based on the benchmark results but I would really like someone from the V8 team to give us a long-term advice for this.
FWIW, v8 themselves have moved away from using JS (https://github.com/v8/v8/blob/d6ead37d265d7215cf9c5f768f279e21bd170212/src/js/prologue.js#L152-L156) to C++ (https://github.com/v8/v8/blob/4b9b23521e6fd42373ebbcb20ebe03bf445494f9/src/bootstrapper.cc#L4619-L4620) to implement uncurryThis. I don't know if that's something we could reuse in node, but that might improve perf even more. |
We can't reuse it in node, and V8 has since removed that code entirely. |
Landed in 4f0f2e7...05847a7 |
This is done to avoid creating an array and gain performance. Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> PR-URL: #36221 Refs: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
This is done to avoid creating an array and gain performance. Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> PR-URL: #36221 Refs: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
This is done to avoid creating an array and gain performance. Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> PR-URL: #36221 Refs: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
This is done to avoid creating an array and gain performance. Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com> PR-URL: #36221 Refs: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
This is done to avoid creating an array and gain performance.
Refs: https://v8.dev/blog/v8-release-80#optimizing-higher-order-builtins
cc @ExE-Boss @mscdex
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes