code review 12662043: net: avoid string operation and make valid domain names... (issue 12662043)

[dr.volke...@gmail.com]

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com (cc: golan...@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/


Description:
net: avoid string operation and make valid domain names explicit

Having a trailing dot in the string doesn't really simplify
the checking loop in isDomainName. Avoid this unnecessary allocation.
Also make the valid domain names more explicit by adding some more
test cases.

benchmark old ns/op new ns/op delta
BenchmarkDNSNames 2420.0 983.0 -59.38%

benchmark old allocs new allocs delta
BenchmarkDNSNames 12 0 -100.00%

benchmark old bytes new bytes delta
BenchmarkDNSNames 336 0 -100.00%

Please review this at https://codereview.appspot.com/12662043/

Affected files:
M src/pkg/net/dnsclient.go
M src/pkg/net/dnsname_test.go


Index: src/pkg/net/dnsclient.go
===================================================================
--- a/src/pkg/net/dnsclient.go
+++ b/src/pkg/net/dnsclient.go
@@ -122,12 +122,9 @@
if len(s) > 255 {
return false
}
- if s[len(s)-1] != '.' { // simplify checking loop: make name end in dot
- s += "."
- }

last := byte('.')
- ok := false // ok once we've seen a letter
+ ok := false // Ok once we've seen a letter.
partlen := 0
for i := 0; i < len(s); i++ {
c := s[i]
@@ -141,13 +138,13 @@
// fine
partlen++
case c == '-':
- // byte before dash cannot be dot
+ // Byte before dash cannot be dot.
if last == '.' {
return false
}
partlen++
case c == '.':
- // byte before dot cannot be dot, dash
+ // Byte before dot cannot be dot, dash.
if last == '.' || last == '-' {
return false
}
@@ -158,6 +155,9 @@
}
last = c
}
+ if last == '-' || partlen > 63 {
+ return false
+ }

return ok
}
Index: src/pkg/net/dnsname_test.go
===================================================================
--- a/src/pkg/net/dnsname_test.go
+++ b/src/pkg/net/dnsname_test.go
@@ -16,7 +16,6 @@
var tests = []testCase{
// RFC2181, section 11.
{"_xmpp-server._tcp.google.com", true},
- {"_xmpp-server._tcp.google.com", true},
{"foo.com", true},
{"1foo.com", true},
{"26.0.0.73.com", true},
@@ -24,6 +23,10 @@
{"fo1o.com", true},
{"foo1.com", true},
{"a.b..com", false},
+ {"a.b-.com", false},
+ {"a.b.com-", false},
+ {"a.b..", false},
+ {"b.com.", true},
}

func getTestCases(ch chan<- testCase) {
@@ -63,3 +66,21 @@
}
}
}
+
+func BenchmarkDNSNames(b *testing.B) {
+ s16 := "0123456789abcdef"
+ s65 := s16 + s16 + s16 + s16
+ s64 := s65[:64]
+ s63 := s65[:63]
+ benchmarks := append(tests, []testCase{
+ {s63, true},
+ {s64, false},
+ }...)
+ for n := 0; n < b.N; n++ {
+ for i := range benchmarks {
+ if isDomainName(benchmarks[i].name) != benchmarks[i].result {
+ b.Fatalf("isDomainName(%q)", benchmarks[i].name)
+ }
+ }
+ }
+}

[brad...@golang.org]

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go
File src/pkg/net/dnsname_test.go (right):

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode71
src/pkg/net/dnsname_test.go:71: s16 := "0123456789abcdef"
drop all this

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode76
src/pkg/net/dnsname_test.go:76: {s63, true},
and just say strings.Repeat("a", 63)

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode80
src/pkg/net/dnsname_test.go:80: for i := range benchmarks {
for i, tt := range benchmarks {

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode81
src/pkg/net/dnsname_test.go:81: if isDomainName(benchmarks[i].name) !=
benchmarks[i].result {
tt.name and tt.result

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode82
src/pkg/net/dnsname_test.go:82: b.Fatalf("isDomainName(%q)",
benchmarks[i].name)
tt.name

https://codereview.appspot.com/12662043/

[dr.volke...@gmail.com]

PTAL

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go
File src/pkg/net/dnsname_test.go (right):

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode71
src/pkg/net/dnsname_test.go:71: s16 := "0123456789abcdef"

On 2013/08/08 18:03:46, bradfitz wrote:
> drop all this

Done.

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode76
src/pkg/net/dnsname_test.go:76: {s63, true},

On 2013/08/08 18:03:46, bradfitz wrote:
> and just say strings.Repeat("a", 63)

Done.

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode80
src/pkg/net/dnsname_test.go:80: for i := range benchmarks {

On 2013/08/08 18:03:46, bradfitz wrote:
> for i, tt := range benchmarks {

Done.

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode81
src/pkg/net/dnsname_test.go:81: if isDomainName(benchmarks[i].name) !=
benchmarks[i].result {

On 2013/08/08 18:03:46, bradfitz wrote:
> tt.name and tt.result

Done.

https://codereview.appspot.com/12662043/diff/4005/src/pkg/net/dnsname_test.go#newcode82
src/pkg/net/dnsname_test.go:82: b.Fatalf("isDomainName(%q)",
benchmarks[i].name)

On 2013/08/08 18:03:46, bradfitz wrote:
> tt.name

Done.

https://codereview.appspot.com/12662043/

[brad...@golang.org]

https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go
File src/pkg/net/dnsname_test.go (right):

https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go#newcode79
src/pkg/net/dnsname_test.go:79: b.Fatalf("isDomainName(%q)", tc.name)
Errorf, not Fatalf.

Also: "isDomainName(%q) = %v; want %v", tc.name, !tc.result, tc.result

https://codereview.appspot.com/12662043/

[dr.volke...@gmail.com]

PTAL

https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go
File src/pkg/net/dnsname_test.go (right):

https://codereview.appspot.com/12662043/diff/1002/src/pkg/net/dnsname_test.go#newcode79
src/pkg/net/dnsname_test.go:79: b.Fatalf("isDomainName(%q)", tc.name)

On 2013/08/08 22:23:26, bradfitz wrote:
> Errorf, not Fatalf.

> Also: "isDomainName(%q) = %v; want %v", tc.name, !tc.result, tc.result

Done. But...

I know: The "got; want" pattern, but isn't this a bit strange
for bools? If the got value is wrong it is pretty clear what the
want value is, or? We are not doing ternary or intuitionistic logic
here.

Fatalf seems to be more common
~/go/src/pkg $ grep b.Fatalf `find . -name "*_test.go"` | wc -l
35
~/go/src/pkg $ $ grep b.Errorf `find . -name "*_test.go"` | wc -l
9
and more logical: There is no point in finishing the benchmark of
a buggy function.

https://codereview.appspot.com/12662043/

[Brad Fitzpatrick]

Let's say I make a change.

I break something.

I'd rather see the 7 cases I broke (and maybe see a pattern of how I broke it, since humans are good at seeing patterns), rather than seeing just one error.

As for the got; want pattern--- if you fail in the same way, I don't have to guess what "foo(%q) doesn't work!" means.  I don't want to go read the test case lines and figure out what the function was supposed to do.

Both points are about saving the debugger (probably not you) time later, by the author (you) paying the cost now.

[Brad Fitzpatrick]

LGTM

[brad...@golang.org]

*** Submitted as
https://code.google.com/p/go/source/detail?r=e70b5c8567b0 ***

net: avoid string operation and make valid domain names explicit

Having a trailing dot in the string doesn't really simplify
the checking loop in isDomainName. Avoid this unnecessary allocation.
Also make the valid domain names more explicit by adding some more
test cases.

benchmark old ns/op new ns/op delta
BenchmarkDNSNames 2420.0 983.0 -59.38%

benchmark old allocs new allocs delta
BenchmarkDNSNames 12 0 -100.00%

benchmark old bytes new bytes delta
BenchmarkDNSNames 336 0 -100.00%

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/12662043

Committer: Brad Fitzpatrick <brad...@golang.org>


https://codereview.appspot.com/12662043/