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

dns: improve makeAsync internally by removing unneeded vaiable. #8800

Merged
merged 1 commit into from
Sep 30, 2016

Conversation

yorkie
Copy link
Contributor

@yorkie yorkie commented Sep 27, 2016

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

dns

Description of change

This removes the unneeded variable inside the MakeCallback's loop. And I have made a benchmark by myself, with 100,000,000 calls and 5 arguments per call, the optimized gets decreased about 500ms.

@nodejs-github-bot nodejs-github-bot added the dns Issues and PRs related to the dns subsystem. label Sep 27, 2016
@Trott
Copy link
Member

Trott commented Sep 27, 2016

A few nits on the commit message:

  • typo:: vaiable - variable
  • please remove the . at the end of the first line of the commit message
  • first line of commit message should not exceed 50 characters

I would suggest:

dns: remove internal variable from makeAsync

@Trott
Copy link
Member

Trott commented Sep 27, 2016

The code that was changed looks like it may have been written that way as a performance optimization. /cc @mscdex

@yorkie
Copy link
Contributor Author

yorkie commented Sep 27, 2016

@Trott do we have a benchmark for that you mean? The following is my bench file:

function callback() {};
function testable() {
  var args = new Array(arguments.length + 1);
  args[0] = callback;
  //for (var i = 0; i < arguments.length; i++)
  //  args[i + 1] = arguments[i];
  for (var i = 1, a = 0; a < arguments.length; i++, a++)
    args[i] = arguments[a];
  return args;
}

var start = Date.now();
for (var i = 0; i < 100000000; i++) testable(1, 2, 3, 4, 5);
var end   = Date.now();
console.log(end - start);

Because Node.js doesn't expose this utility function, thus I have to rewrite once for this tests, correct me if I'm wrong, thanks :)

@jasnell
Copy link
Member

jasnell commented Sep 27, 2016

LGTM

@yorkie
Copy link
Contributor Author

yorkie commented Sep 27, 2016

@lpinca
Copy link
Member

lpinca commented Sep 27, 2016

LGTM

@Trott
Copy link
Member

Trott commented Sep 27, 2016

@yorkie asked:

@Trott do we have a benchmark for that you mean?

Sorry, I missed that you had a benchmark in the first place!

@yorkie
Copy link
Contributor Author

yorkie commented Sep 28, 2016

@Trott Never mind, now this change LGTY? I hope a review from you :)

PR-URL: nodejs#8800
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@yorkie yorkie merged commit 7bc6aea into nodejs:master Sep 30, 2016
@yorkie yorkie deleted the dns/improve-dns branch September 30, 2016 02:06
@yorkie
Copy link
Contributor Author

yorkie commented Sep 30, 2016

Landed in 7bc6aea, thanks :)

jasnell pushed a commit that referenced this pull request Oct 6, 2016
PR-URL: #8800
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
PR-URL: #8800
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants