fs: ES6 Spread Operator for extended parameter handling #17486

mithunsasidharan · 2017-12-06T11:46:52Z

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

benjamingr · 2017-12-06T12:37:18Z

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

apapirovski

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

mithunsasidharan · 2017-12-06T12:51:27Z

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

benjamingr · 2017-12-06T14:01:13Z

@apapirovski would still love some benchmarks

apapirovski · 2017-12-06T14:12:26Z

@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 · 2017-12-06T14:16:20Z

@apapirovski : Thanks for sharing that. Quite helpful 👍

benjamingr · 2017-12-06T16:46:40Z

@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 · 2017-12-06T16:48:42Z

@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 · 2017-12-06T16:53:15Z

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 · 2017-12-06T16:57:17Z

@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 · 2017-12-06T17:00:40Z

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 · 2017-12-06T17:09:33Z

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 · 2017-12-06T17:28:18Z

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

Outdated

mithunsasidharan · 2017-12-06T17:32:11Z

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

apapirovski · 2017-12-06T17:32:07Z

lib/fs.js

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

-  return function() {
-    return cb.apply(undefined, arguments);
+  return function(...arguments) {


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

@apapirovski : my bad.. will fix that !

apapirovski · 2017-12-06T17:34:11Z

lib/fs.js

-  return function() {
-    return cb.apply(undefined, arguments);
+  return function(...arguments) {
+    return cb.apply(undefined, args);


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.

@apapirovski : Please take a look now. Thanks !

bmeurer · 2017-12-06T17:46:44Z

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

apapirovski

LGTM if CI & benchmark CI are happy.

apapirovski · 2017-12-06T17:48:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11915/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/71/

mithunsasidharan · 2017-12-06T17:48:48Z

@apapirovski : Thanks much !

benjamingr · 2017-12-06T21:50:40Z

LGTM

apapirovski · 2017-12-08T18:45:19Z

Landed in 0a0fbd5

PR-URL: #17486 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

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

apapirovski previously requested changes Dec 6, 2017

View reviewed changes

mithunsasidharan closed this Dec 6, 2017

mithunsasidharan reopened this Dec 6, 2017

apapirovski reviewed Dec 6, 2017

View reviewed changes

fs: ES6 spread operator for better gain

98398e5

apapirovski approved these changes Dec 6, 2017

View reviewed changes

benjamingr approved these changes Dec 6, 2017

View reviewed changes

apapirovski mentioned this pull request Dec 7, 2017

fs: use Reflect.apply #17514

Closed

2 tasks

cjihrig approved these changes Dec 7, 2017

View reviewed changes

bmeurer approved these changes Dec 7, 2017

View reviewed changes

TimothyGu approved these changes Dec 8, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:38

apapirovski closed this Dec 8, 2017

apapirovski added the refactor to ES6+ label Dec 8, 2017

benjamingr added dont-land-on-v4.x labels Dec 10, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

mithunsasidharan mentioned this pull request Dec 13, 2017

cluster: use rest param & Reflect.apply #17655

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: ES6 Spread Operator for extended parameter handling #17486

fs: ES6 Spread Operator for extended parameter handling #17486

mithunsasidharan commented Dec 6, 2017 •

edited

Loading

benjamingr commented Dec 6, 2017

apapirovski left a comment

mithunsasidharan commented Dec 6, 2017

benjamingr commented Dec 6, 2017

apapirovski commented Dec 6, 2017

mithunsasidharan commented Dec 6, 2017

benjamingr commented Dec 6, 2017

apapirovski commented Dec 6, 2017

apapirovski commented Dec 6, 2017

mithunsasidharan commented Dec 6, 2017 •

edited

Loading

apapirovski commented Dec 6, 2017

apapirovski commented Dec 6, 2017 •

edited

Loading

apapirovski commented Dec 6, 2017

mithunsasidharan commented Dec 6, 2017

apapirovski Dec 6, 2017

mithunsasidharan Dec 6, 2017

apapirovski Dec 6, 2017

mithunsasidharan Dec 6, 2017

bmeurer commented Dec 6, 2017

apapirovski left a comment

apapirovski commented Dec 6, 2017

mithunsasidharan commented Dec 6, 2017

benjamingr commented Dec 6, 2017

apapirovski commented Dec 8, 2017

fs: ES6 Spread Operator for extended parameter handling #17486

fs: ES6 Spread Operator for extended parameter handling #17486

Conversation

mithunsasidharan commented Dec 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

benjamingr commented Dec 6, 2017

apapirovski left a comment

Choose a reason for hiding this comment

mithunsasidharan commented Dec 6, 2017

benjamingr commented Dec 6, 2017

apapirovski commented Dec 6, 2017

mithunsasidharan commented Dec 6, 2017

benjamingr commented Dec 6, 2017

apapirovski commented Dec 6, 2017

apapirovski commented Dec 6, 2017

mithunsasidharan commented Dec 6, 2017 • edited Loading

apapirovski commented Dec 6, 2017

apapirovski commented Dec 6, 2017 • edited Loading

apapirovski commented Dec 6, 2017

mithunsasidharan commented Dec 6, 2017

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

mithunsasidharan Dec 6, 2017

Choose a reason for hiding this comment

apapirovski Dec 6, 2017

Choose a reason for hiding this comment

mithunsasidharan Dec 6, 2017

Choose a reason for hiding this comment

bmeurer commented Dec 6, 2017

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski commented Dec 6, 2017

mithunsasidharan commented Dec 6, 2017

benjamingr commented Dec 6, 2017

apapirovski commented Dec 8, 2017

mithunsasidharan commented Dec 6, 2017 •

edited

Loading

mithunsasidharan commented Dec 6, 2017 •

edited

Loading

apapirovski commented Dec 6, 2017 •

edited

Loading