Issue 128820043: code review 128820043: net: improve behavior of unix native Go DNS queries
Descriptionnet: implement query-response fast failover in builtin dns stub resolver
Speed improvements via code cleanup, and changes to make go dns behave more like glibc resolver. See https://groups.google.com/forum/#!topic/golang-dev/lV-0aHqxVeo
Fixes issue 6579.
Benchmark results on linux/amd64
benchmark old ns/op new ns/op delta
BenchmarkGoLookupIP 4831903 2572937 -46.75%
BenchmarkGoLookupIPNoSuchHost 10114105 2419641 -76.08%
BenchmarkGoLookupIPWithBrokenNameServer 20007735624 5004490730 -74.99%
benchmark old allocs new allocs delta
BenchmarkGoLookupIP 287 288 0.35%
BenchmarkGoLookupIPNoSuchHost 204 102 -50.00%
BenchmarkGoLookupIPWithBrokenNameServer 410 358 -12.68%
benchmark old bytes new bytes delta
BenchmarkGoLookupIP 13181 13271 0.68%
BenchmarkGoLookupIPNoSuchHost 17260 8714 -49.51%
BenchmarkGoLookupIPWithBrokenNameServer 28160 22432 -20.34%
Patch Set 1 #Patch Set 2 : diff -r fc86fb1cc0f7c4dce5b78c2ab3d00b6723b7b8fc https://code.google.com/p/go #Patch Set 3 : diff -r fc86fb1cc0f7c4dce5b78c2ab3d00b6723b7b8fc https://code.google.com/p/go #Patch Set 4 : diff -r fc86fb1cc0f7c4dce5b78c2ab3d00b6723b7b8fc https://code.google.com/p/go #
Total comments: 16
Patch Set 5 : diff -r 68b84694821cc0b6a22405bc0c73e73c80e17ca8 https://code.google.com/p/go #
Total comments: 8
Patch Set 6 : diff -r 277f98d885558a00e899f6e89958e97470fd8ab4 https://code.google.com/p/go #
Total comments: 6
Patch Set 7 : diff -r 173175ba9eb71f00f69da09c738523eb4fab36a6 https://code.google.com/p/go #Patch Set 8 : diff -r 173175ba9eb71f00f69da09c738523eb4fab36a6 https://code.google.com/p/go #Patch Set 9 : diff -r 173175ba9eb71f00f69da09c738523eb4fab36a6 https://code.google.com/p/go #
Total comments: 2
Patch Set 10 : diff -r 173175ba9eb71f00f69da09c738523eb4fab36a6 https://code.google.com/p/go #
Total comments: 1
Patch Set 11 : diff -r e5c87cefb57fffc5767209542be5c4e58123e3c6 https://code.google.com/p/go #Patch Set 12 : diff -r e5c87cefb57fffc5767209542be5c4e58123e3c6 https://code.google.com/p/go #Patch Set 13 : diff -r e5c87cefb57fffc5767209542be5c4e58123e3c6 https://code.google.com/p/go #
MessagesTotal messages: 21
Hello golang-codereviews@googlegroups.com (cc: mikioh.mikioh@gmail.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:174: server += ":53" use JoinHostPort https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:184: } else { if err != nil { lastErr = ... if nerr, ok := err.(Error); ok && nerr.Timeout() { lastErr.IsTimeout = true } continue } https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:306: if rooted || err == nil { is this related to issue 6579? if not, please put this in a separate cl https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:325: if count(name, '.') < cfg.dnsConfig.ndots { ditto https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:384: go goLookupConcurrent(name, dnsTypeA, ipv4chan) do we really need a non-anonymous function? https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:386: addr4 := <-ipv4chan while we are here, let's use more readable variable names; e.g., addr for literal address and ip for net.IP https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:392: if len(addrs) > 0 || (addr4.err == nil && addr6.err == nil) { drop unnecessary parens https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsconfig_uni... File src/pkg/net/dnsconfig_unix.go (right): https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsconfig_uni... src/pkg/net/dnsconfig_unix.go:106: if len(conf.servers) == 0 { s this related to issue 6579? if not, please put this in a separate cl
Sign in to reply to this message.
Hi, PTAL. Thanks, Alex https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:174: server += ":53" On 2014/08/20 12:46:50, mikio wrote: > use JoinHostPort Done. https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:184: } else { On 2014/08/20 12:46:50, mikio wrote: > if err != nil { > lastErr = ... > if nerr, ok := err.(Error); ok && nerr.Timeout() { > lastErr.IsTimeout = true > } > continue > } I tried this initially, but because lastErr is defined as type error, you cannot set the IsTimeout field this way. Should we introduce a new scoped variable or leave it? https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:306: if rooted || err == nil { On 2014/08/20 12:46:50, mikio wrote: > is this related to issue 6579? if not, please put this in a separate cl By itself, this particular line isn't. I needed to work on this section(see next comment) and this was an easy 3 line cleanup, not sure it's worth its own CL but if it bothers you, I can certainly revert it. https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:325: if count(name, '.') < cfg.dnsConfig.ndots { On 2014/08/20 12:46:50, mikio wrote: > ditto See above. A nonrooted query with dots > ndots is tried twice if any error. This includes dead ns, timeout, and even nosuchhost. While the redoing of attempts/servers was the main culprit, this certainly compounds it. https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:384: go goLookupConcurrent(name, dnsTypeA, ipv4chan) On 2014/08/20 12:46:50, mikio wrote: > do we really need a non-anonymous function? Done. I originally did this, but found it caused allocs to jump slightly(2-5%), but not certain why. I went ahead and went back to using anonymous funcs as you imply and a waitgroup. Thoughts on this and the alloc increase welcome. https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:386: addr4 := <-ipv4chan On 2014/08/20 12:46:50, mikio wrote: > while we are here, let's use more readable variable names; e.g., addr for > literal address and ip for net.IP Done. https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:392: if len(addrs) > 0 || (addr4.err == nil && addr6.err == nil) { On 2014/08/20 12:46:50, mikio wrote: > drop unnecessary parens Done. https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsconfig_uni... File src/pkg/net/dnsconfig_unix.go (right): https://codereview.appspot.com/128820043/diff/60001/src/pkg/net/dnsconfig_uni... src/pkg/net/dnsconfig_unix.go:106: if len(conf.servers) == 0 { On 2014/08/20 12:46:50, mikio wrote: > s this related to issue 6579? if not, please put this in a separate cl Agreed, this is not related to the issue. We can do it later.
Sign in to reply to this message.
Mikio, Josh, could you review? The tree closes in a few days.
Sign in to reply to this message.
> Mikio, Josh, could you review? Here are a few superficial comments. I don't have the background to assess beyond that. https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:324: // that is - name is not rooted and has less than ndots dots. s/ - /, / https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:387: defer lwg.Done() May as well skip the cost of the defer and just call Done after lookup. https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:391: defer lwg.Done() Same. https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:395: addrs = append(convertRR_A(aRecords), convertRR_AAAA(aaaaRecords)...) Can you construct addrs after checking for errors?
Sign in to reply to this message.
Thanks Josh, made suggested edits if you want to peek again. Mikio? Thanks, Alex https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:324: // that is - name is not rooted and has less than ndots dots. On 2014/08/28 16:24:30, josharian wrote: > s/ - /, / Done. https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:387: defer lwg.Done() On 2014/08/28 16:24:30, josharian wrote: > May as well skip the cost of the defer and just call Done after lookup. Done. https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:391: defer lwg.Done() On 2014/08/28 16:24:30, josharian wrote: > Same. Done. https://codereview.appspot.com/128820043/diff/80001/src/pkg/net/dnsclient_uni... src/pkg/net/dnsclient_unix.go:395: addrs = append(convertRR_A(aRecords), convertRR_AAAA(aaaaRecords)...) On 2014/08/28 16:24:30, josharian wrote: > Can you construct addrs after checking for errors? The first error check could be moved up, so I've done it. The others must stay below.
Sign in to reply to this message.
On 2014/08/28 15:40:41, bradfitz wrote: > Mikio, Josh, could you review? will try this afternoon. > The tree closes in a few days. i think the martial law doesn't affect bugfixes, am i missing something?
Sign in to reply to this message.
thanks. can you please change the cl descr something like; implement query-response fast failover in builtin dns stub resolver https://codereview.appspot.com/128820043/diff/100001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/128820043/diff/100001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:176: if err != nil { you miss that lastErr is just an interface and we can do type assertion. if err != nil { lastErr = &DNSError{ : } if nerr, ok := err.(Error); ok && nerr.Timeout() { lastErr.(*DNSError).IsTimeout = true } continue } https://codereview.appspot.com/128820043/diff/100001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:325: if count(name, '.') < cfg.dnsConfig.ndots { sorry i missed this change in previous review. can you please make a separate cl for this change even if we still don't support partial/absolute lookup using search-list+ndots in builtin stub resolver. https://codereview.appspot.com/128820043/diff/100001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:381: } you can write this like the following: type racer struct { qtype uint16 rrs []dnsRR error } lane := make(chan racer, 1) qtypes := []uint16{dnsTypeA, dnsTypeAAAA} for _, qtype := range qtypes { go func(qtype uint16) { _, rrs, err := lookup(name, qtype) lane <- racer{qtype, rrs, err} }(qtype) } var lastErr error for range qtypes { racer := <-lane if racer.error != nil { lastErr = racer.error continue } switch racer.qtype { case dnsTypeA: addrs = append(addrs, convertRR_A(racer.rrs)...) case dnsTypeAAAA: addrs = append(addrs, convertRR_AAAA(racer.rrs)...) } } if lastErr != nil { return nil, lastErr } return addrs, nil }
Sign in to reply to this message.
Thanks. Made recommended changes and will move suggested logic cleanup and associated test into another CL. PTAL. Thanks, Alex https://codereview.appspot.com/128820043/diff/100001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/128820043/diff/100001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:176: if err != nil { On 2014/08/29 14:33:00, mikio wrote: > you miss that lastErr is just an interface and we can do type assertion. > > if err != nil { > lastErr = &DNSError{ > : > } > if nerr, ok := err.(Error); ok && nerr.Timeout() { > lastErr.(*DNSError).IsTimeout = true > } > continue > } Done. https://codereview.appspot.com/128820043/diff/100001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:325: if count(name, '.') < cfg.dnsConfig.ndots { On 2014/08/29 14:33:00, mikio wrote: > sorry i missed this change in previous review. can you please make a separate cl > for this change even if we still don't support partial/absolute lookup using > search-list+ndots in builtin stub resolver. Done. I put the broken code back and will open new issue/CL. https://codereview.appspot.com/128820043/diff/100001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:381: } On 2014/08/29 14:33:00, mikio wrote: > you can write this like the following: > > type racer struct { > qtype uint16 > rrs []dnsRR > error > } > lane := make(chan racer, 1) > qtypes := []uint16{dnsTypeA, dnsTypeAAAA} > for _, qtype := range qtypes { > go func(qtype uint16) { > _, rrs, err := lookup(name, qtype) > lane <- racer{qtype, rrs, err} > }(qtype) > } > var lastErr error > for range qtypes { > racer := <-lane > if racer.error != nil { > lastErr = racer.error > continue > } > switch racer.qtype { > case dnsTypeA: > addrs = append(addrs, convertRR_A(racer.rrs)...) > case dnsTypeAAAA: > addrs = append(addrs, convertRR_AAAA(racer.rrs)...) > } > } > if lastErr != nil { > return nil, lastErr > } > return addrs, nil > } Done with two caveats. First, this cause 20% jump in allocs in test cases. I've been trying to push this change for a year now, so I'm not going to argue against it, just an FYI. Second - the old logic returned addresses if they existed, so I tried to mimic that by using if len(addrs) == 0 && lastErr != nil. If you are against this prior behavior we can change, but why even have a lastErr when we can just bail out if the first query errors?
Sign in to reply to this message.
https://codereview.appspot.com/128820043/diff/160001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/128820043/diff/160001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:385: lane := make(chan racer, 1) If you're going to return early, you need to have a big enough buffer for all goroutines to send, on pain of leaking. So move this down a line and change to lane := make(chan racer, len(qtypes)) (That is assuming that returning early is the right thing to do, which I don't know.)
Sign in to reply to this message.
Hi, PTAL. Mikio/Josh - please see below, your thoughts on hopefully this last blocker very welcome. Thanks, Alex https://codereview.appspot.com/128820043/diff/160001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/128820043/diff/160001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:385: lane := make(chan racer, 1) On 2014/08/29 22:47:48, josharian wrote: > If you're going to return early, you need to have a big enough buffer for all > goroutines to send, on pain of leaking. > > So move this down a line and change to > > lane := make(chan racer, len(qtypes)) > > (That is assuming that returning early is the right thing to do, which I don't > know.) Done. Thanks for this. My only goal here is scenario where query one returns noSuchHost immediately, and query 2 was dropped or otherwise timed out. No need to wait on query 2 in this scenario, so it would be nice to return early. If however this introduces possibility of leak, I will revert to lastErr method suggested. Thoughts?
Sign in to reply to this message.
> If however this introduces possibility of leak, I will revert to lastErr method > suggested. Thoughts? It shouldn't leak now. As for whether the behavior is correct, I defer to Mikio. Would you mind updating this to apply cleanly against tip? Thanks. And thanks again for your perseverance with this. https://codereview.appspot.com/128820043/diff/180001/src/pkg/net/dnsclient_un... File src/pkg/net/dnsclient_unix.go (right): https://codereview.appspot.com/128820043/diff/180001/src/pkg/net/dnsclient_un... src/pkg/net/dnsclient_unix.go:385: qtypes := []uint16{dnsTypeA, dnsTypeAAAA} Nit: Use [...]uint16, so len(qtypes) will be a compile-time constant.
Sign in to reply to this message.
On Friday, August 29, 2014 4:04:00 PM UTC-7, Alex Skinner wrote: > My only goal here is scenario where query one returns noSuchHost > immediately, and query 2 was dropped or otherwise timed out. No need to > wait on query 2 in this scenario, so it would be nice to return early. The previous behavior is preferable: ignore errors if any lookups succeed. As it is currently, it will only succeed if a domain has both A and AAAA records. If it is missing either type, then this will return a noSuchHost error.
Sign in to reply to this message.
On 2014/08/29 18:16:30, axaxs wrote: > Done with two caveats. First, this cause 20% jump in allocs in test cases. > I've been trying to push this change for a year now, so I'm not going to argue > against it, just an FYI. sure, but we can avoid tons of internal dns encoding/decoding helpers that make unnecessary allocations later, maybe in go1.5.
Sign in to reply to this message.
Absolutely right! I was just complaining about this recently on a golang-dev thread. Thank you very much, fixed. Mikio - Can we fix this in 1.5? IMO, an auth answer for a missing type should not be a no such host. Only NXDOMAIN should. Thoughts? Thanks, Alex On Fri, Aug 29, 2014 at 9:21 PM, <abursavich@gmail.com> wrote: > On Friday, August 29, 2014 4:04:00 PM UTC-7, Alex Skinner wrote: > > My only goal here is scenario where query one returns noSuchHost > > immediately, and query 2 was dropped or otherwise timed out. No need to > > wait on query 2 in this scenario, so it would be nice to return early. > > The previous behavior is preferable: ignore errors if any lookups > succeed. As it is currently, it will only succeed if a domain has both A > and AAAA records. If it is missing either type, then this will return a > noSuchHost error.
Sign in to reply to this message.
I think(hope) it is finalized now after a mix of edits from advice from Mikio, Josh, and abursavich. PTAL. Thanks, Alex On Fri, Aug 29, 2014 at 9:40 PM, <mikioh.mikioh@gmail.com> wrote: > On 2014/08/29 18:16:30, axaxs wrote: > > Done with two caveats. First, this cause 20% jump in allocs in test >> > cases. > >> I've been trying to push this change for a year now, so I'm not going >> > to argue > >> against it, just an FYI. >> > > sure, but we can avoid tons of internal dns encoding/decoding helpers > that make unnecessary allocations later, maybe in go1.5. > > > https://codereview.appspot.com/128820043/ >
Sign in to reply to this message.
On Sat, Aug 30, 2014 at 10:53 AM, Alex Skinner <alex@lx.lc> wrote: > Mikio - Can we fix this in 1.5? IMO, an auth answer for a missing type > should not be a no such host. Only NXDOMAIN should. Thoughts? i'm happy if you have a plan for refactoring builtin dns stub resolver comprehensively in go1.5.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
On 2014/08/30 02:31:18, mikio wrote: > LGTM will submit after lunch
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=a32faf936e45 *** net: implement query-response fast failover in builtin dns stub resolver Speed improvements via code cleanup, and changes to make go dns behave more like glibc resolver. See https://groups.google.com/forum/#!topic/golang-dev/lV-0aHqxVeo Fixes issue 6579. Benchmark results on linux/amd64 benchmark old ns/op new ns/op delta BenchmarkGoLookupIP 4831903 2572937 -46.75% BenchmarkGoLookupIPNoSuchHost 10114105 2419641 -76.08% BenchmarkGoLookupIPWithBrokenNameServer 20007735624 5004490730 -74.99% benchmark old allocs new allocs delta BenchmarkGoLookupIP 287 288 0.35% BenchmarkGoLookupIPNoSuchHost 204 102 -50.00% BenchmarkGoLookupIPWithBrokenNameServer 410 358 -12.68% benchmark old bytes new bytes delta BenchmarkGoLookupIP 13181 13271 0.68% BenchmarkGoLookupIPNoSuchHost 17260 8714 -49.51% BenchmarkGoLookupIPWithBrokenNameServer 28160 22432 -20.34% LGTM=mikioh.mikioh R=golang-codereviews, mikioh.mikioh, bradfitz, josharian, abursavich CC=golang-codereviews https://codereview.appspot.com/128820043 Committer: Mikio Hara <mikioh.mikioh@gmail.com>
Sign in to reply to this message.
This CL appears to have broken the windows-amd64-perf builder. See http://build.golang.org/log/00c238ffcf2b69bebca242d0f56172d7dbc97016
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
