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

net: dnsConfig could be more usable as-is #15473

Closed
mdempsky opened this issue Apr 27, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@mdempsky
Copy link
Member

commented Apr 27, 2016

dnsConfig currently reflects the contents of /etc/resolv.conf, but that means some of the fields need to be repeatedly processed in dnsclient_unix.go before being used. For example, in tryOneName we have to call JoinHostPort every time for each queried server, producing extra garbage, or convert cfg.timeout to time.Duration. (*dnsConfig).nameList could be simplified if we ensured search list entries always ended in a period. Maybe there are more.

/cc @dpiddy @bradfitz

@mdempsky mdempsky added this to the Go1.7Maybe milestone Apr 27, 2016

@danp

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2016

Sounds good. How about I start with changing cfg.timeout to a duration?

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2016

SGTM

@gopherbot

This comment has been minimized.

Copy link

commented Apr 28, 2016

CL https://golang.org/cl/22548 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 28, 2016

net: change type of dnsConfig.timeout from int to time.Duration
Instead of keeping the desired number of seconds and converting to
time.Duration for every query, convert to time.Duration when
building the config.

Updates #15473

Change-Id: Ib24c050b593b3109011e359f4ed837a3fb45dc65
Reviewed-on: https://go-review.googlesource.com/22548
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Apr 28, 2016

CL https://golang.org/cl/22556 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 28, 2016

CL https://golang.org/cl/22571 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 28, 2016

net: append ":53" to DNS servers when reading resolv.conf
Avoids generating some redundant garbage from re-concatenating the
same string for every DNS query.

benchmark                                      old allocs     new allocs     delta
BenchmarkGoLookupIP-32                         156            154            -1.28%
BenchmarkGoLookupIPNoSuchHost-32               456            446            -2.19%
BenchmarkGoLookupIPWithBrokenNameServer-32     577            564            -2.25%

benchmark                                      old bytes     new bytes     delta
BenchmarkGoLookupIP-32                         10873         10824         -0.45%
BenchmarkGoLookupIPNoSuchHost-32               43303         43140         -0.38%
BenchmarkGoLookupIPWithBrokenNameServer-32     46824         46616         -0.44%

Update #15473.

Change-Id: I3b0173dfedf31bd08eaea1069968b416850864a1
Reviewed-on: https://go-review.googlesource.com/22556
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot pushed a commit that referenced this issue Apr 28, 2016

net: ensure dnsConfig search list is rooted
Avoids some extra work and string concatenation at query time.

benchmark                                      old allocs     new allocs     delta
BenchmarkGoLookupIP-32                         154            150            -2.60%
BenchmarkGoLookupIPNoSuchHost-32               446            442            -0.90%
BenchmarkGoLookupIPWithBrokenNameServer-32     564            568            +0.71%

benchmark                                      old bytes     new bytes     delta
BenchmarkGoLookupIP-32                         10824         10704         -1.11%
BenchmarkGoLookupIPNoSuchHost-32               43140         42992         -0.34%
BenchmarkGoLookupIPWithBrokenNameServer-32     46616         46680         +0.14%

BenchmarkGoLookupIPWithBrokenNameServer's regression appears to be
because it's actually only performing 1 LookupIP call, so the extra
work done parsing the DNS config file doesn't amortize as well as for
BenchmarkGoLookupIP or BenchmarkGoLOokupIPNoSuchHost, which perform
2000+ LookupIP calls per run.

Update #15473.

Change-Id: I98c8072f2f39e2f2ccd6c55e9e9bd309f5ad68f8
Reviewed-on: https://go-review.googlesource.com/22571
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2016

This is as done as I had in mind. Can reopen or file new issues if anyone has ideas for more work.

@mdempsky mdempsky closed this Apr 28, 2016

@golang golang locked and limited conversation to collaborators Apr 28, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.