dns: refactor internal/dns/promises.js #27081

Trott · 2019-04-04T00:09:59Z

Use isIP() instead of isIPv4() since it does the additional
functionality that we were adding after our calls to isIP().

This not-so-incidentally also increases code coverage from tests. At
least one of the replaced ternaries was difficult to cover reliably
because operating system/configuration variances were too unpredictable.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Use `isIP()` instead of `isIPv4()` since it does the additional functionality that we were adding after our calls to `isIP()`. This not-so-incidentally also increases code coverage from tests. At least one of the replaced ternaries was difficult to cover reliably because operating system/configuration variances were too unpredictable.

nodejs-github-bot · 2019-04-04T00:10:01Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3163

nodejs-github-bot · 2019-04-04T00:10:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22184/

richardlau · 2019-04-04T00:22:44Z

Use isIP() instead of isIPv4() since it does the additional
functionality that we were adding after our calls to isIP().

FWIW isIP() does do an extra regex test in the IPv6 case. Is this a hot path? Do we have any relevant benchmarks?

Trott · 2019-04-04T00:48:11Z

FWIW isIP() does do an extra regex test in the IPv6 case.

Yeah, I guess it's worth noting that, at least in theory, this can change the behavior. Previously, anything that wasn't IPv4 was automatically deemed IPv6. Now we're actually checking that the address is a legit IPv6, and if it's not, family will be 0. However, in practice, that shouldn't happen. Moreover, if there is a malformed address, I don't think throwing up our hands and calling it IPv6 is the correct behavior. That seems like a bug. Using 0 instead at least makes some sense. Throwing an error might be warranted but even if we decide to go that route, I'd rather do incremental change anyway.

Is this a hot path? Do we have any relevant benchmarks?

Both of these are in callbacks for lookup() so it seems doubtful that it's a hot path. But we do have a lookup benchmark, so let's run it.

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/318/

richardlau · 2019-04-04T00:59:27Z

Both of these are in callbacks for lookup() so it seems doubtful that it's a hot path. But we do have a lookup benchmark, so let's run it.

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/318/

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/319/

Trott · 2019-04-04T04:38:29Z

Results seem OK to me so far. Will try with a larger 'N' to see if we can get something statistically significant.

18:13:55                                                       confidence improvement accuracy (*)   (**)  (***)
18:13:55  dns/lookup.js n=5000000 all='false' name=''                         -0.06 %       ±2.95% ±3.93% ±5.11%
18:13:55  dns/lookup.js n=5000000 all='false' name='::1'                      -0.83 %       ±1.75% ±2.32% ±3.02%
18:13:55  dns/lookup.js n=5000000 all='false' name='127.0.0.1'                -0.30 %       ±2.04% ±2.71% ±3.53%
18:13:55  dns/lookup.js n=5000000 all='true' name=''                          -1.08 %       ±2.92% ±3.89% ±5.07%
18:13:55  dns/lookup.js n=5000000 all='true' name='::1'                       -0.74 %       ±2.79% ±3.74% ±4.93%
18:13:55  dns/lookup.js n=5000000 all='true' name='127.0.0.1'                 -0.50 %       ±4.57% ±6.09% ±7.93%
18:13:55 
18:13:55 Be aware that when doing many comparisons the risk of a false-positive
18:13:55 result increases. In this case there are 6 comparisons, you can thus
18:13:55 expect the following amount of false-positive results:
18:13:55   0.30 false positives, when considering a   5% risk acceptance (*, **, ***),
18:13:55   0.06 false positives, when considering a   1% risk acceptance (**, ***),
18:13:55   0.01 false positives, when considering a 0.1% risk acceptance (***)
18:13:56 ++ mv output040419-020556.csv /w/bnch-comp

Trott · 2019-04-04T04:40:07Z

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/320/ (Same as previous run but bumping the number of times we run each benchmark up from 30 to 128 in the hopes of getting some statistically significant results. Alternatively, if we see some results that show this PR being faster, it will be more obvious that this is a wash.)

Trott · 2019-04-04T05:42:09Z

Hmmm....

22:21:41 ++ Rscript benchmark/compare.R
22:21:41                                                       confidence improvement accuracy (*)   (**)  (***)
22:21:41  dns/lookup.js n=5000000 all='false' name=''                          0.24 %       ±1.51% ±1.98% ±2.55%
22:21:41  dns/lookup.js n=5000000 all='false' name='::1'                       0.62 %       ±0.91% ±1.20% ±1.54%
22:21:41  dns/lookup.js n=5000000 all='false' name='127.0.0.1'        ***     -4.38 %       ±1.66% ±2.19% ±2.81%
22:21:41  dns/lookup.js n=5000000 all='true' name=''                           1.02 %       ±1.72% ±2.27% ±2.91%
22:21:41  dns/lookup.js n=5000000 all='true' name='::1'                       -0.00 %       ±1.08% ±1.42% ±1.82%
22:21:41  dns/lookup.js n=5000000 all='true' name='127.0.0.1'                 -0.55 %       ±1.97% ±2.60% ±3.33%
22:21:41 
22:21:41 Be aware that when doing many comparisons the risk of a false-positive
22:21:41 result increases. In this case there are 6 comparisons, you can thus
22:21:41 expect the following amount of false-positive results:
22:21:41   0.30 false positives, when considering a   5% risk acceptance (*, **, ***),
22:21:41   0.06 false positives, when considering a   1% risk acceptance (**, ***),
22:21:41   0.01 false positives, when considering a 0.1% risk acceptance (***)

Trott · 2019-04-04T05:43:30Z

Running again for confirmation: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/321/

richardlau · 2019-04-04T05:53:54Z

It occurs to me that the benchmark probably isn't actually testing the code touched by this PR as it tests the non-promise version of dns.lookup() 😆.

Having said that, there is similar code in

node/lib/dns.js

Line 63 in a3cf96c

this.callback(null, addresses[0], isIPv4(addresses[0]) ? 4 : 6);

and

node/lib/dns.js

Line 78 in a3cf96c

family: family || (isIPv4(addr) ? 4 : 6)

which we should probably keep consistent.

Trott · 2019-04-04T13:19:05Z

You sure the current benchmark isn't affected by this change already?:

23:26:01                                                       confidence improvement accuracy (*)   (**)  (***)
23:26:01  dns/lookup.js n=5000000 all='false' name=''                          0.12 %       ±1.55% ±2.04% ±2.62%
23:26:01  dns/lookup.js n=5000000 all='false' name='::1'                      -0.15 %       ±1.05% ±1.39% ±1.78%
23:26:01  dns/lookup.js n=5000000 all='false' name='127.0.0.1'        ***     -2.44 %       ±1.27% ±1.68% ±2.15%
23:26:01  dns/lookup.js n=5000000 all='true' name=''                          -0.39 %       ±1.80% ±2.37% ±3.04%
23:26:01  dns/lookup.js n=5000000 all='true' name='::1'                       -0.12 %       ±1.44% ±1.90% ±2.44%
23:26:01  dns/lookup.js n=5000000 all='true' name='127.0.0.1'                 -0.36 %       ±2.02% ±2.66% ±3.41%
23:26:01 
23:26:01 Be aware that when doing many comparisons the risk of a false-positive
23:26:01 result increases. In this case there are 6 comparisons, you can thus
23:26:01 expect the following amount of false-positive results:
23:26:01   0.30 false positives, when considering a   5% risk acceptance (*, **, ***),
23:26:01   0.06 false positives, when considering a   1% risk acceptance (**, ***),
23:26:01   0.01 false positives, when considering a 0.1% risk acceptance (***)

Trott · 2019-04-04T13:19:55Z

(I agree that it doesn't make sense. But at the same time, the results are consistent. Something's up.)

BridgeAR · 2019-04-04T14:54:54Z

@Trott this looks like statistical anomalies to me.

richardlau · 2019-04-04T15:49:29Z

You sure the current benchmark isn't affected by this change already?:

Yes. At least according to coverage:

-bash-4.2$ rm -rf out/Release/.coverage/
-bash-4.2$ NODE_V8_COVERAGE=out/Release/.coverage tools/test.py test/benchmark/test-benchmark-dns.js
[00:00|% 100|+   1|-   0]: Done
-bash-4.2$ grep -oP '[\w/.]*dns[\w/.]*' out/Release/.coverage/*
out/Release/.coverage/coverage-43652-1554392808703-0.json:dns.js
out/Release/.coverage/coverage-43666-1554392808663-0.json:///home/users/riclau/sandbox/github/nodejs/benchmark/dns/lookup.js
out/Release/.coverage/coverage-43666-1554392808663-0.json:dns.js
out/Release/.coverage/coverage-43666-1554392808663-0.json:internal/dns/utils.js
out/Release/.coverage/coverage-43676-1554392808645-0.json:///home/users/riclau/sandbox/github/nodejs/benchmark/dns/lookup.js
out/Release/.coverage/coverage-43676-1554392808645-0.json:dns.js
out/Release/.coverage/coverage-43676-1554392808645-0.json:internal/dns/utils.js
-bash-4.2$

Trott · 2019-04-04T16:21:08Z

@Trott this looks like statistical anomalies to me.

But the results are consistent. Twice now, the same one has a very high degree of confidence (low p-value). While it all-but-certainly isn't a reflection of the code change, it is (almost certainly) a reflection of something. An artifact somewhere. Maybe with this benchmark, and the way the operating system caches lookups, the second benchmark will always be slower? In other words, maybe the benchmark doesn't yield valid results ever, at least on the OS where we're running it?

Let's test that theory: Here's a run with no code changes. (The PR is a change to a GOVERNANCE.md only, so the code source should be identical.) https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/322/

Trott · 2019-04-04T17:03:48Z

Ah, there we go, the benchmark is clearly not providing valid results. Same code shows statistically significant differences in performance:

10:02:54 ++ Rscript benchmark/compare.R
10:02:54                                                       confidence improvement accuracy (*)   (**)  (***)
10:02:54  dns/lookup.js n=5000000 all='false' name=''                          0.67 %       ±1.50% ±1.98% ±2.54%
10:02:54  dns/lookup.js n=5000000 all='false' name='::1'                       0.90 %       ±1.08% ±1.42% ±1.82%
10:02:54  dns/lookup.js n=5000000 all='false' name='127.0.0.1'         **     -1.97 %       ±1.23% ±1.62% ±2.08%
10:02:54  dns/lookup.js n=5000000 all='true' name=''                           1.17 %       ±1.68% ±2.22% ±2.84%
10:02:54  dns/lookup.js n=5000000 all='true' name='::1'                        0.29 %       ±1.00% ±1.32% ±1.70%
10:02:54  dns/lookup.js n=5000000 all='true' name='127.0.0.1'                 -0.82 %       ±2.01% ±2.65% ±3.40%
10:02:54 
10:02:54 Be aware that when doing many comparisons the risk of a false-positive
10:02:54 result increases. In this case there are 6 comparisons, you can thus
10:02:54 expect the following amount of false-positive results:
10:02:54   0.30 false positives, when considering a   5% risk acceptance (*, **, ***),
10:02:54   0.06 false positives, when considering a   1% risk acceptance (**, ***),
10:02:54   0.01 false positives, when considering a 0.1% risk acceptance (***)

The benchmarks for dns.lookup() include calling it with an empty hostname which results in a deprecation warning. This benchmark seems to be subject to some odd side effects (see Ref below) and we probably generally don't want to benchmark deprecated things by default anyway. Remove the deprecated value from the default list. Bonus is that this will speed up the benchmark. Refs: nodejs#27081 (comment)

The benchmarks for dns.lookup() include calling it with an empty hostname which results in a deprecation warning. This benchmark seems to be subject to some odd side effects (see Ref below) and we probably generally don't want to benchmark deprecated things by default anyway. Remove the deprecated value from the default list. Bonus is that this will speed up the benchmark. Refs: nodejs#27081 (comment) PR-URL: nodejs#27091 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Trott · 2019-04-07T05:41:26Z

OK, the benchmark has been modified slightly, so let's see if we no longer get the false positive on a running-the-same-version-of-node-for-both-benchmarks: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/326/

And, in an optimistic move, let's re-run the benchmark for this PR: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/327/ (scheduled)

If that doesn't have surprising results, then I suppose the next step is to write a benchmark for DNS promises and to add the isIP() change to the callback version, and re-run benchmarks.

Trott · 2019-04-07T06:20:08Z

Still seeing the false positive in the benchmark run using the same executable:

23:14:21                                                       confidence improvement accuracy (*)   (**)  (***)
23:14:21  dns/lookup.js n=5000000 all='false' name='::1'                       0.46 %       ±0.85% ±1.13% ±1.45%
23:14:21  dns/lookup.js n=5000000 all='false' name='127.0.0.1'          *     -1.83 %       ±1.66% ±2.18% ±2.80%
23:14:21  dns/lookup.js n=5000000 all='true' name='::1'                       -0.44 %       ±0.96% ±1.26% ±1.62%
23:14:21  dns/lookup.js n=5000000 all='true' name='127.0.0.1'                  0.49 %       ±2.19% ±2.88% ±3.70%
23:14:21 
23:14:21 Be aware that when doing many comparisons the risk of a false-positive
23:14:21 result increases. In this case there are 4 comparisons, you can thus
23:14:21 expect the following amount of false-positive results:
23:14:21   0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
23:14:21   0.04 false positives, when considering a   1% risk acceptance (**, ***),
23:14:21   0.00 false positives, when considering a 0.1% risk acceptance (***)

BridgeAR · 2019-04-09T14:44:51Z

@Trott if you change the cases mentioned here #27081 (comment) and then restart the benchmark, it'll execute changed code in the expected way. So the outcome should also be more significant. One star as confidence is mostly a statistical anomaly. Yes, there were also three stars with a low percentage value but even that happens from time to time.

Trott · 2019-04-16T06:16:13Z

Running this change with the benchmark file in #27249 resulted in no statistically-significant difference in performance. I think this is good to land.

$ cat /var/tmp/trott | Rscript benchmark/compare.R 
                                                               confidence improvement accuracy (*)   (**)  (***)
 dns/lookup-promises.js n=5000000 all='false' name='::1'                       0.63 %       ±3.40% ±4.53% ±5.90%
 dns/lookup-promises.js n=5000000 all='false' name='127.0.0.1'                -1.68 %       ±2.72% ±3.63% ±4.74%
 dns/lookup-promises.js n=5000000 all='true' name='::1'                       -0.47 %       ±2.32% ±3.10% ±4.04%
 dns/lookup-promises.js n=5000000 all='true' name='127.0.0.1'                  2.53 %       ±3.48% ±4.64% ±6.08%
 dns/lookup.js n=5000000 all='false' name='::1'                                0.56 %       ±3.38% ±4.50% ±5.85%
 dns/lookup.js n=5000000 all='false' name='127.0.0.1'                          2.14 %       ±4.92% ±6.55% ±8.54%
 dns/lookup.js n=5000000 all='true' name='::1'                                 2.47 %       ±3.17% ±4.22% ±5.51%
 dns/lookup.js n=5000000 all='true' name='127.0.0.1'                          -0.96 %       ±4.11% ±5.47% ±7.12%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 8 comparisons, you can thus
expect the following amount of false-positive results:
  0.40 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.08 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)
$

BridgeAR

@Trott it would be good to actually have a consistent handling across the "regular" dns functions and the promise based ones as pointed out here #27081 (comment). Please do not land this for promises only.

Trott · 2019-04-16T18:10:27Z

@Trott it would be good to actually have a consistent handling across the "regular" dns functions and the promise based ones as pointed out here #27081 (comment). Please do not land this for promises only.

Done. A re-review or two would be good. @jasnell @lpinca @richardlau @BridgeAR

In lib/dns.js, use `isIP()` instead of `isIPv4()` for determining the `family` property in `lookup()`. If an invalid IP address is returned, the `family` currently provided is `6`. With this change, it will be `0`. Update documentation to reflect this.

nodejs-github-bot · 2019-04-16T18:12:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22509/

Trott · 2019-04-16T20:23:47Z

Additional benchmark for the callback-based changes. Might not actually be relevant to the change here, but hey, for completeness....

                                                      confidence improvement accuracy (*)   (**)   (***)
 dns/lookup.js n=5000000 all='false' name='::1'                      -1.36 %       ±2.45% ±3.27%  ±4.25%
 dns/lookup.js n=5000000 all='false' name='127.0.0.1'                 2.22 %       ±3.62% ±4.81%  ±6.27%
 dns/lookup.js n=5000000 all='true' name='::1'                        1.37 %       ±3.38% ±4.51%  ±5.90%
 dns/lookup.js n=5000000 all='true' name='127.0.0.1'                  2.94 %       ±7.22% ±9.61% ±12.51%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 4 comparisons, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

Trott · 2019-04-16T23:22:20Z

Landed in 14df42f...09cdc37

Use `isIP()` instead of `isIPv4()` since it does the additional functionality that we were adding after our calls to `isIP()`. This not-so-incidentally also increases code coverage from tests. At least one of the replaced ternaries was difficult to cover reliably because operating system/configuration variances were too unpredictable. PR-URL: nodejs#27081 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

In lib/dns.js, use `isIP()` instead of `isIPv4()` for determining the `family` property in `lookup()`. If an invalid IP address is returned, the `family` currently provided is `6`. With this change, it will be `0`. Update documentation to reflect this. PR-URL: nodejs#27081 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Adding this benchmark will let us check the performance implications of nodejs#27081.

Adding this benchmark will let us check the performance implications of nodejs#27081. PR-URL: nodejs#27249 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>

nodejs-github-bot added the dns Issues and PRs related to the dns subsystem. label Apr 4, 2019

richardlau approved these changes Apr 4, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2019

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2019

Trott mentioned this pull request Apr 4, 2019

benchmark: remove deprecated argument #27091

Closed

4 tasks

lpinca approved these changes Apr 6, 2019

View reviewed changes

jasnell approved these changes Apr 8, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2019

BridgeAR reviewed Apr 16, 2019

View reviewed changes

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2019

Trott force-pushed the refactor-dns-promises branch from 9670302 to d3c7421 Compare April 16, 2019 18:11

BridgeAR approved these changes Apr 16, 2019

View reviewed changes

richardlau approved these changes Apr 16, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 16, 2019

Trott closed this Apr 16, 2019

Trott added a commit to Trott/io.js that referenced this pull request Apr 18, 2019

benchmark: add benchmark for dns.promises.lookup()

57a9ad9

Adding this benchmark will let us check the performance implications of nodejs#27081.

Trott mentioned this pull request Apr 28, 2019

Benchmarks in CI produce erroneous results nodejs/build#1793

Closed

Trott deleted the refactor-dns-promises branch January 13, 2022 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: refactor internal/dns/promises.js #27081

dns: refactor internal/dns/promises.js #27081

Trott commented Apr 4, 2019

nodejs-github-bot commented Apr 4, 2019

nodejs-github-bot commented Apr 4, 2019

richardlau commented Apr 4, 2019 •

edited

Loading

Trott commented Apr 4, 2019

richardlau commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

richardlau commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

BridgeAR commented Apr 4, 2019

richardlau commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 7, 2019

Trott commented Apr 7, 2019

BridgeAR commented Apr 9, 2019

Trott commented Apr 16, 2019

BridgeAR left a comment

Trott commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019

Trott commented Apr 16, 2019 •

edited

Loading

Trott commented Apr 16, 2019

dns: refactor internal/dns/promises.js #27081

dns: refactor internal/dns/promises.js #27081

Conversation

Trott commented Apr 4, 2019

Checklist

nodejs-github-bot commented Apr 4, 2019

nodejs-github-bot commented Apr 4, 2019

richardlau commented Apr 4, 2019 • edited Loading

Trott commented Apr 4, 2019

richardlau commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

richardlau commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

BridgeAR commented Apr 4, 2019

richardlau commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 4, 2019

Trott commented Apr 7, 2019

Trott commented Apr 7, 2019

BridgeAR commented Apr 9, 2019

Trott commented Apr 16, 2019

BridgeAR left a comment

Choose a reason for hiding this comment

Trott commented Apr 16, 2019

nodejs-github-bot commented Apr 16, 2019

Trott commented Apr 16, 2019 • edited Loading

Trott commented Apr 16, 2019

richardlau commented Apr 4, 2019 •

edited

Loading

Trott commented Apr 16, 2019 •

edited

Loading