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: fix issues in dns benchmark #14936

Closed
wants to merge 2 commits into from

Conversation

ian-perkins
Copy link
Contributor

@ian-perkins ian-perkins commented Aug 19, 2017

The benchmark script for dns contained functions with args declared
but never used. This fix removes those arguments from the function
signatures.

No test existed for the dns benchmark so one was added to the
parallel suite.

The dns benchmark uses the core dns.lookup() function which does not
access the network but instead uses "an operating system facility
that can associate names with addresses, and vice versa" e.g. similar
to ping; however, it is a synchronous call which runs on the libuv
threadpool - the number of test calls was therefore reduced to 5e4
(from 5e6).

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)

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. dns Issues and PRs related to the dns subsystem. test Issues and PRs related to the tests. labels Aug 19, 2017
{ NODEJS_BENCHMARK_ZERO_ALLOWED: 1 });

const child = fork(runjs,
['--set', 'n=5e4',
Copy link
Member

Choose a reason for hiding this comment

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

We just want to make sure the thing runs so I think this could be n=1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add:

'--set', 'all=false',
'--set', 'name=127.0.0.1',

so as to minimize the breadth of the benchmark even further.

@Trott
Copy link
Member

Trott commented Aug 19, 2017

The most important fix is that this changes the script from one that doesn't work to one that can actually run. :-D

@Trott
Copy link
Member

Trott commented Aug 19, 2017

Before this commit:

$ ./node benchmark/run.js dns

dns/lookup.js
/Users/trott/io.js/benchmark/common.js:80
        throw new TypeError(`configuration "${key}" had type ${typeof value}`);
        ^

TypeError: configuration "all" had type boolean
    at recursive (/Users/trott/io.js/benchmark/common.js:80:15)
    at recursive (/Users/trott/io.js/benchmark/common.js:91:9)
    at Benchmark._queue (/Users/trott/io.js/benchmark/common.js:99:5)
    at new Benchmark (/Users/trott/io.js/benchmark/common.js:18:21)
    at Object.exports.createBenchmark (/Users/trott/io.js/benchmark/common.js:7:10)
    at Object.<anonymous> (/Users/trott/io.js/benchmark/dns/lookup.js:6:22)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
$

After this commit, it actually runs.

@Trott
Copy link
Member

Trott commented Aug 19, 2017

And after:

$ ./node benchmark/run.js dns

dns/lookup.js
dns/lookup.js n=5000000 all="true" name="": 4,485,516.774544048
dns/lookup.js n=5000000 all="false" name="": 4,614,725.86397749
dns/lookup.js n=5000000 all="true" name="127.0.0.1": 2,519,219.886801595
dns/lookup.js n=5000000 all="false" name="127.0.0.1": 2,523,608.1753037865
dns/lookup.js n=5000000 all="true" name="::1": 2,633,747.082621402
dns/lookup.js n=5000000 all="false" name="::1": 2,705,813.055286177
$

🎉 Thanks, @ian-perkins!

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. Welcome @ian-perkins and thanks.

@Trott
Copy link
Member

Trott commented Aug 19, 2017

Tiny nit: The subsystem here should likely be benchmark rather than dns. If you don't mind changing it in the commit message, you'll save whoever lands this a few keystrokes.

@Trott
Copy link
Member

Trott commented Aug 19, 2017

@@ -5,7 +5,7 @@ const lookup = require('dns').lookup;

const bench = common.createBenchmark(main, {
name: ['', '127.0.0.1', '::1'],
all: [true, false],
all: ['true', 'false'],
Copy link
Member

Choose a reason for hiding this comment

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

This actually breaks the intention of the Boolean. The value will be coerced to a Boolean again in line 15 and !!'false' === true. So you might just change this to [1, 0] instead.

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR Oh, good catch.

Copy link
Member

@Trott Trott Aug 19, 2017

Choose a reason for hiding this comment

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

Another possibility is to leave them as strings but change the assignment to something like: const all = conf.all === 'true' ? true : false;

To me, this has the advantage of being more readable and also seems to be at least a little bit in line with what some other benchmarks do. Of course, @BridgeAR's suggestion works too if that seems preferable.

Trott
Trott previously requested changes Aug 19, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Need to address the !!'false' === true thing noted by @BridgeAR. LGTM otherwise.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % strings && 5e4 comments addressed

@BridgeAR
Copy link
Member

@ian-perkins thanks a lot for following up on the comments. I must ask you to rebase though as described here as we can otherwise not cleanly apply your changes. Keep up the good work!

@Trott Trott dismissed their stale review August 23, 2017 23:27

Looks like requests have been addressed, clearing my request for changes.

@Trott
Copy link
Member

Trott commented Aug 23, 2017

@BridgeAR Looks like this has no conflicts with master anymore. PTAL.

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

hmm..looks like the conflicts were resolved by merging and not rebasing.

@Trott
Copy link
Member

Trott commented Aug 24, 2017

hmm..looks like the conflicts were resolved by merging and not rebasing.

Ah! Yes, extra commits, now I see. Yeah, ignore me, and @ian-perkins please rebase when you get a chance. :-D

@ian-perkins
Copy link
Contributor Author

I'll try to do this today but can I first check what the rebase should do? Is it to squash all the commits I made into one? I read the referred doc and it mentions commits being squashed on landing so I suspect that I need to do something different in this case? Sorry for the basic nature of my questions...

@Trott
Copy link
Member

Trott commented Aug 24, 2017

@ian-perkins The rebase is to synch up your branch with the master branch on the upstream repository, and then re-apply your changes on top of that. (Hope that answers your question, but if not, feel free to ask clarifying questions here or hop on IRC to see if someone can help you!)

The benchmark script for dns contained functions with args declared
but never used. This fix removes those arguments from the function
signatures.

No test existed for the dns benchmark so one was added to the
parallel suite.

The dns benchmark uses the core dns.lookup() function which does not
access the network but instead uses "an operating system facility
that can associate names with addresses, and vice versa" e.g. similar
to ping; however, it is a synchronous call which runs on the libuv
threadpool - the number of test calls was therefore reduced to 5e4
(from 5e6).
The benchmark script for dns contained functions with args declared
but never used. This fix removes those arguments from the function
signatures.

No test existed for the dns benchmark so one was added to the
parallel suite.

The dns benchmark uses the core dns.lookup() function which does not
access the network.

To improve performance the tests are limited to 1 invocation to a
single endpoint.
@ian-perkins
Copy link
Contributor Author

I did the rebase and pushed to my fork - hopefully it's ok...

@Trott
Copy link
Member

Trott commented Aug 24, 2017

@BridgeAR PTAL

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Aug 24, 2017

@BridgeAR BridgeAR self-assigned this Aug 25, 2017
BridgeAR pushed a commit that referenced this pull request Aug 25, 2017
The benchmark script for dns contained functions with args declared
but never used. This fix removes those arguments from the function
signatures.

No test existed for the dns benchmark so one was added to the
parallel suite.
To improve performance the tests are limited to 1 invocation to a
single endpoint.

PR-URL: #14936
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@BridgeAR
Copy link
Member

Landed in 15ac1ea

@ian-perkins congratulations on your first contribution! 🎉

@BridgeAR BridgeAR closed this Aug 25, 2017
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
The benchmark script for dns contained functions with args declared
but never used. This fix removes those arguments from the function
signatures.

No test existed for the dns benchmark so one was added to the
parallel suite.
To improve performance the tests are limited to 1 invocation to a
single endpoint.

PR-URL: nodejs/node#14936
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
The benchmark script for dns contained functions with args declared
but never used. This fix removes those arguments from the function
signatures.

No test existed for the dns benchmark so one was added to the
parallel suite.
To improve performance the tests are limited to 1 invocation to a
single endpoint.

PR-URL: nodejs/node#14936
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
The benchmark script for dns contained functions with args declared
but never used. This fix removes those arguments from the function
signatures.

No test existed for the dns benchmark so one was added to the
parallel suite.
To improve performance the tests are limited to 1 invocation to a
single endpoint.

PR-URL: #14936
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
The benchmark script for dns contained functions with args declared
but never used. This fix removes those arguments from the function
signatures.

No test existed for the dns benchmark so one was added to the
parallel suite.
To improve performance the tests are limited to 1 invocation to a
single endpoint.

PR-URL: #14936
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. dns Issues and PRs related to the dns subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants