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

http: use direct parameters instead #10833

Closed
wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

@JacksonTian JacksonTian commented Jan 16, 2017

When parameter count is fixed, use literal Array instance is more
simply and avoid arguments leak also.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

http

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jan 16, 2017
};

ClientRequest.prototype.setSocketKeepAlive = _setSocketKeepAlive;
Copy link
Contributor

@mscdex mscdex Jan 16, 2017

Choose a reason for hiding this comment

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

I think the underscore should be removed here, so that the function name matches the prototype property name, just like with setNoDelay() above.

@mscdex
Copy link
Contributor

mscdex commented Jan 16, 2017

Nit: saying the arguments were being leaked is a bit misleading, it makes it sound as if the arguments object was being passed to something other than a .apply(). I think it would be more accurate to say something like 'avoid copying arguments with a loop.'

caculate([noDelay]);
}

var common = require('../common.js');
Copy link
Contributor

@mscdex mscdex Jan 16, 2017

Choose a reason for hiding this comment

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

This should go at the top of the file, after 'use strict';. I would probably also put the var bench = ... after this var common = ... line at the top as well.

@simonkcleung
Copy link

simonkcleung commented Jan 16, 2017

Basically, it doesnt work.

'use strict';

var o={x:0};

function caculate(args) {
  o.x=args[0]+args[1] + 1;
}

function setNoDelay() {
  const argsLen = arguments.length;
  const args = new Array(argsLen);
  for (var i = 0; i < argsLen; i++)
    args[i] = arguments[i];
  caculate(args);
}

function setNoDelay2(noDelay) {
  caculate([noDelay]);
}

var common = require('../common.js');

var bench = common.createBenchmark(main, {
  n: [1000000],
  name: ['setNoDelay', 'setNoDelay2']
}, {});

function main(conf) {
  var n = conf.n;
  var method = conf.name === 'setNoDelay' ? setNoDelay : setNoDelay2;
  var i;

  bench.start();

  for (i = 0; i < n; i++) {
    method(1,2);
  }
  console.log(o.x);
  bench.end(n);
}

Difference!

@JacksonTian
Copy link
Contributor Author

@simonkcleung the benchmark test case is for copy arguments item vs literal Array instance. When parameters count is 2, should change the setNoDelay and setNoDelay2 too.

@simonkcleung
Copy link

simonkcleung commented Jan 17, 2017

@JacksonTian Right! I think I overlooked something.
In the API, request.setNoDelay([noDelay]), there is only 1 or 0 parameter. It is no need to use the for looping.

@JacksonTian JacksonTian force-pushed the args branch 2 times, most recently from 0888be0 to 3dd353a Compare January 18, 2017 03:44
@JacksonTian
Copy link
Contributor Author

I updated the commits by review suggestion. Would like to review it again? @mscdex

@mscdex
Copy link
Contributor

mscdex commented Jan 19, 2017

I'm not sure the benchmark is worth including really. I don't think there will ever be a case when the loop will ever be faster than the explicit array literal case.

@JacksonTian
Copy link
Contributor Author

@mscdex Let me remove the benchmark.

When parameter count is fixed, use literal Array instance is more
simply and avoid arguments leak also.
@mscdex
Copy link
Contributor

mscdex commented Jan 20, 2017

LGTM if CI is ok with it: https://ci.nodejs.org/job/node-test-pull-request/5960/

@italoacasas
Copy link
Contributor

Landed aa8eb87

italoacasas pushed a commit that referenced this pull request Jan 31, 2017
When parameter count is fixed, use literal Array instance is more
simply and avoid arguments leak also.

PR-URL: #10833
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
When parameter count is fixed, use literal Array instance is more
simply and avoid arguments leak also.

PR-URL: #10833
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

Would require a backport PR if it should land on v6 or v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants