From 5666663370f2939c8705bf1a73aa88c0114877f9 Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Tue, 18 May 2021 12:46:30 -0700 Subject: [PATCH] net/packet: use netaddr AppendTo methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This lets us remote the types/strbuilder package, which had only a single user. And it's faster. name old time/op new time/op delta String/tcp4-8 175ns ± 0% 58ns ± 1% -66.95% (p=0.000 n=10+9) String/tcp6-8 226ns ± 1% 136ns ± 1% -39.85% (p=0.000 n=10+10) String/udp4-8 175ns ± 1% 58ns ± 1% -67.01% (p=0.000 n=10+9) String/udp6-8 230ns ± 1% 140ns ± 0% -39.32% (p=0.000 n=10+9) String/icmp4-8 164ns ± 0% 50ns ± 1% -69.89% (p=0.000 n=10+10) String/icmp6-8 217ns ± 1% 129ns ± 0% -40.46% (p=0.000 n=10+10) String/igmp-8 196ns ± 0% 56ns ± 1% -71.32% (p=0.000 n=10+10) String/unknown-8 2.06ns ± 1% 2.06ns ± 2% ~ (p=0.985 n=10+10) name old alloc/op new alloc/op delta String/tcp4-8 32.0B ± 0% 32.0B ± 0% ~ (all equal) String/tcp6-8 168B ± 0% 96B ± 0% -42.86% (p=0.000 n=10+10) String/udp4-8 32.0B ± 0% 32.0B ± 0% ~ (all equal) String/udp6-8 168B ± 0% 96B ± 0% -42.86% (p=0.000 n=10+10) String/icmp4-8 32.0B ± 0% 32.0B ± 0% ~ (all equal) String/icmp6-8 104B ± 0% 64B ± 0% -38.46% (p=0.000 n=10+10) String/igmp-8 48.0B ± 0% 48.0B ± 0% ~ (all equal) String/unknown-8 0.00B 0.00B ~ (all equal) name old allocs/op new allocs/op delta String/tcp4-8 1.00 ± 0% 1.00 ± 0% ~ (all equal) String/tcp6-8 3.00 ± 0% 1.00 ± 0% -66.67% (p=0.000 n=10+10) String/udp4-8 1.00 ± 0% 1.00 ± 0% ~ (all equal) String/udp6-8 3.00 ± 0% 1.00 ± 0% -66.67% (p=0.000 n=10+10) String/icmp4-8 1.00 ± 0% 1.00 ± 0% ~ (all equal) String/icmp6-8 3.00 ± 0% 1.00 ± 0% -66.67% (p=0.000 n=10+10) String/igmp-8 1.00 ± 0% 1.00 ± 0% ~ (all equal) String/unknown-8 0.00 0.00 ~ (all equal) Signed-off-by: Josh Bleecher Snyder --- cmd/tailscale/depaware.txt | 1 - cmd/tailscaled/depaware.txt | 1 - net/packet/packet.go | 42 +++++----------- types/strbuilder/strbuilder.go | 74 ----------------------------- types/strbuilder/strbuilder_test.go | 52 -------------------- 5 files changed, 11 insertions(+), 159 deletions(-) delete mode 100644 types/strbuilder/strbuilder.go delete mode 100644 types/strbuilder/strbuilder_test.go diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 89f722e8b..74d3e9e1e 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -48,7 +48,6 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep tailscale.com/types/opt from tailscale.com/net/netcheck+ tailscale.com/types/persist from tailscale.com/ipn tailscale.com/types/preftype from tailscale.com/cmd/tailscale/cli+ - tailscale.com/types/strbuilder from tailscale.com/net/packet tailscale.com/types/structs from tailscale.com/ipn+ tailscale.com/types/wgkey from tailscale.com/types/netmap+ tailscale.com/util/dnsname from tailscale.com/cmd/tailscale/cli+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 47e94f4ab..7e934e198 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -128,7 +128,6 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/types/opt from tailscale.com/control/controlclient+ tailscale.com/types/persist from tailscale.com/control/controlclient+ tailscale.com/types/preftype from tailscale.com/ipn+ - tailscale.com/types/strbuilder from tailscale.com/net/packet tailscale.com/types/structs from tailscale.com/control/controlclient+ tailscale.com/types/wgkey from tailscale.com/control/controlclient+ L tailscale.com/util/cmpver from tailscale.com/net/dns diff --git a/net/packet/packet.go b/net/packet/packet.go index 7bb31aa5f..5c27b6cc4 100644 --- a/net/packet/packet.go +++ b/net/packet/packet.go @@ -12,7 +12,6 @@ import ( "inet.af/netaddr" "tailscale.com/types/ipproto" - "tailscale.com/types/strbuilder" ) const unknown = ipproto.Unknown @@ -62,36 +61,17 @@ func (p *Parsed) String() string { return "Unknown{???}" } - sb := strbuilder.Get() - sb.WriteString(p.IPProto.String()) - sb.WriteByte('{') - writeIPPort(sb, p.Src) - sb.WriteString(" > ") - writeIPPort(sb, p.Dst) - sb.WriteByte('}') - return sb.String() -} - -// writeIPPort writes ipp.String() into sb, with fewer allocations. -// -// TODO: make netaddr more efficient in this area, and retire this func. -func writeIPPort(sb *strbuilder.Builder, ipp netaddr.IPPort) { - if ipp.IP().Is4() { - raw := ipp.IP().As4() - sb.WriteUint(uint64(raw[0])) - sb.WriteByte('.') - sb.WriteUint(uint64(raw[1])) - sb.WriteByte('.') - sb.WriteUint(uint64(raw[2])) - sb.WriteByte('.') - sb.WriteUint(uint64(raw[3])) - sb.WriteByte(':') - } else { - sb.WriteByte('[') - sb.WriteString(ipp.IP().String()) // TODO: faster? - sb.WriteString("]:") - } - sb.WriteUint(uint64(ipp.Port())) + // max is the maximum reasonable length of the string we are constructing. + // It's OK to overshoot, as the temp buffer is allocated on the stack. + const max = len("ICMPv6{[ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff%enp5s0]:65535 > [ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff%enp5s0]:65535}") + b := make([]byte, 0, max) + b = append(b, p.IPProto.String()...) + b = append(b, '{') + b = p.Src.AppendTo(b) + b = append(b, ' ', '>', ' ') + b = p.Dst.AppendTo(b) + b = append(b, '}') + return string(b) } // Decode extracts data from the packet in b into q. diff --git a/types/strbuilder/strbuilder.go b/types/strbuilder/strbuilder.go deleted file mode 100644 index 071c03178..000000000 --- a/types/strbuilder/strbuilder.go +++ /dev/null @@ -1,74 +0,0 @@ -// Copyright (c) 2020 Tailscale Inc & AUTHORS All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// Package strbuilder defines a string builder type that allocates -// less than the standard library's strings.Builder by using a -// sync.Pool, so it doesn't matter if the compiler can't prove that -// the builder doesn't escape into the fmt package, etc. -package strbuilder - -import ( - "bytes" - "strconv" - "sync" -) - -var pool = sync.Pool{ - New: func() interface{} { return new(Builder) }, -} - -type Builder struct { - bb bytes.Buffer - scratch [20]byte // long enough for MinInt64, MaxUint64 - locked bool // in pool, not for use -} - -// Get returns a new or reused string Builder. -func Get() *Builder { - b := pool.Get().(*Builder) - b.bb.Reset() - b.locked = false - return b -} - -// String both returns the Builder's string, and returns the builder -// to the pool. -func (b *Builder) String() string { - if b.locked { - panic("String called twiced on Builder") - } - s := b.bb.String() - b.locked = true - pool.Put(b) - return s -} - -func (b *Builder) WriteByte(v byte) error { - return b.bb.WriteByte(v) -} - -func (b *Builder) WriteString(s string) (int, error) { - return b.bb.WriteString(s) -} - -func (b *Builder) Write(p []byte) (int, error) { - return b.bb.Write(p) -} - -func (b *Builder) WriteInt(v int64) { - b.Write(strconv.AppendInt(b.scratch[:0], v, 10)) -} - -func (b *Builder) WriteUint(v uint64) { - b.Write(strconv.AppendUint(b.scratch[:0], v, 10)) -} - -// Grow grows the buffer's capacity, if necessary, to guarantee space -// for another n bytes. After Grow(n), at least n bytes can be written -// to the buffer without another allocation. If n is negative, Grow -// will panic. If the buffer can't grow it will panic with -// ErrTooLarge. -func (b *Builder) Grow(n int) { - b.bb.Grow(n) -} diff --git a/types/strbuilder/strbuilder_test.go b/types/strbuilder/strbuilder_test.go deleted file mode 100644 index f21d8afac..000000000 --- a/types/strbuilder/strbuilder_test.go +++ /dev/null @@ -1,52 +0,0 @@ -// Copyright (c) 2020 Tailscale Inc & AUTHORS All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package strbuilder - -import ( - "math" - "testing" -) - -func TestBuilder(t *testing.T) { - const want = "Hello, world 123 -456!" - bang := []byte("!") - var got string - allocs := testing.AllocsPerRun(1000, func() { - sb := Get() - sb.WriteString("Hello, world ") - sb.WriteUint(123) - sb.WriteByte(' ') - sb.WriteInt(-456) - sb.Write(bang) - got = sb.String() - }) - if got != want { - t.Errorf("got %q; want %q", got, want) - } - if allocs != 1 { - t.Errorf("allocs = %v; want 1", allocs) - } -} - -// Verifies scratch buf is large enough. -func TestIntBounds(t *testing.T) { - const want = "-9223372036854775808 9223372036854775807 18446744073709551615" - var got string - allocs := testing.AllocsPerRun(1000, func() { - sb := Get() - sb.WriteInt(math.MinInt64) - sb.WriteByte(' ') - sb.WriteInt(math.MaxInt64) - sb.WriteByte(' ') - sb.WriteUint(math.MaxUint64) - got = sb.String() - }) - if got != want { - t.Errorf("got %q; want %q", got, want) - } - if allocs != 1 { - t.Errorf("allocs = %v; want 1", allocs) - } -}