Skip to content
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

fs: ES6 Spread Operator for extended parameter handling #17486

Closed
wants to merge 1 commit into from
Closed

fs: ES6 Spread Operator for extended parameter handling #17486

wants to merge 1 commit into from

Conversation

mithunsasidharan
Copy link
Contributor

@mithunsasidharan mithunsasidharan commented Dec 6, 2017

Replaced f.apply(undefined, arguments) to ES 6 f(...arguments) Extended Parameter Handling Spread Operator in lib/fs.js

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Dec 6, 2017
@benjamingr
Copy link
Member

These sort of changes need benchmarks - have you tried running the benchmarks and getting results?

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but I'm strongly -1 on this change. This will be much slower than using apply is.

@mithunsasidharan
Copy link
Contributor Author

@apapirovski : Thanks for the feedback. I'll close the PR.

@benjamingr
Copy link
Member

@apapirovski would still love some benchmarks

@apapirovski
Copy link
Member

@benjamingr I'm speaking from experience after trying to make this change on events and having the benchmarks be roughly 20-25% slower in certain situations. Theoretically this is supposed to be optimized (a basic benchmark shows maybe 2-3% perf gap and that might just be noise), practically there seem to be edge cases.

@mithunsasidharan
Copy link
Contributor Author

@apapirovski : Thanks for sharing that. Quite helpful 👍

@benjamingr
Copy link
Member

@apapirovski can you send me a link to those benchmarks then? This is the third time we've been talking about whether or not spread is optimized or not and I want something concrete to go with when I bug V8 people about it :)

@apapirovski
Copy link
Member

@benjamingr the benchmarks are just the events ones... I think I saw the issue when working on once or something similar. It's also possible it might've been in process.nextTick or timers. I can try to reproduce again when I have a moment.

@apapirovski
Copy link
Member

I wish I had more info, it's just that I've been working on that stuff on-and-off for the past 2 months so it's a bit hard to recall at what point I encountered it. You could look over recent events, timers & process PRs potentially.

@mithunsasidharan
Copy link
Contributor Author

mithunsasidharan commented Dec 6, 2017

@apapirovski @benjamingr : Just to mention, post closing the PR.. over my conversation with few JS experienced dev folks.. they did acknowledge too that f.apply(undefined, arguments); is much better performant than f(...arguments); in multiple different scenarios and they did confirm they've tested it !

@apapirovski
Copy link
Member

ping @bmeurer any thoughts on fn(...arguments) vs fn.apply(undefined, arguments)? I'm almost certain I've run into edge cases before but I also know the V8 team has done work to optimize this recently?

@apapirovski
Copy link
Member

apapirovski commented Dec 6, 2017

FWIW I think the fastest combination here might be

function(...args) {
  return cb.apply(undefined, args);
}

When I test that version within once with ee-once benchmark, it's about 5% faster than using arguments. Don't ask me why...

@mithunsasidharan If you would like to try that version of the PR and run the relevant benchmarks in benchmark/fs then that PR could probably be accepted?

Here's the bench results after a short run:

fs/bench-readdir.js n=10000      1.95 %        *** 0.0006178907

Reasonably certain it's faster. Don't think it can land on v8.x or older though.

@apapirovski
Copy link
Member

Also, you could potentially test fn(...args) { return cb(...args); }? It might be the same. Not sure.

@mithunsasidharan
Copy link
Contributor Author

@apapirovski : Can you take a look at updated change here ?

lib/fs.js Outdated
@@ -145,8 +145,8 @@ function makeCallback(cb) {
throw new errors.TypeError('ERR_INVALID_CALLBACK');
}

return function() {
return cb.apply(undefined, arguments);
return function(...arguments) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ...args to match the variable below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski : my bad.. will fix that !

lib/fs.js Outdated
return function() {
return cb.apply(undefined, arguments);
return function(...arguments) {
return cb.apply(undefined, args);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On that note, could we change this to Reflect.apply(cb, undefined, args);? Thank you! That will mean we no longer depend on the user provided apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski : Please take a look now. Thanks !

@bmeurer
Copy link
Member

bmeurer commented Dec 6, 2017

@apapirovski Yes I also think fn.apply(undefined, args) is best for now.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI & benchmark CI are happy.

@mithunsasidharan
Copy link
Contributor Author

@apapirovski : Thanks much !

@benjamingr
Copy link
Member

LGTM

@apapirovski apapirovski mentioned this pull request Dec 7, 2017
2 tasks
@apapirovski
Copy link
Member

Landed in 0a0fbd5

@apapirovski apapirovski closed this Dec 8, 2017
apapirovski pushed a commit that referenced this pull request Dec 8, 2017
PR-URL: #17486
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17486
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17486
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants