From 61e31ed48be78bd5cb2a57d1e2806ce5d25298e7 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 1 Oct 2018 15:39:57 -0700 Subject: [PATCH 1/6] stop copying when calling `Bytes` This is a huge performance hit. Really, we just need to tell users not to modify the result. Also, get rid of an unnecessary pointer indirection (no api change). --- interface.go | 2 ++ multiaddr.go | 31 +++++++++++++++---------------- multiaddr_test.go | 6 ------ util.go | 6 +++--- 4 files changed, 20 insertions(+), 25 deletions(-) diff --git a/interface.go b/interface.go index c68e4b4..1f46184 100644 --- a/interface.go +++ b/interface.go @@ -18,6 +18,8 @@ type Multiaddr interface { Equal(Multiaddr) bool // Bytes returns the []byte representation of this Multiaddr + // + // This function may expose immutable, internal state. Do not modify. Bytes() []byte // String returns the string representation of this Multiaddr diff --git a/multiaddr.go b/multiaddr.go index 098e5ed..9b5c251 100644 --- a/multiaddr.go +++ b/multiaddr.go @@ -24,7 +24,7 @@ func NewMultiaddr(s string) (a Multiaddr, err error) { if err != nil { return nil, err } - return &multiaddr{bytes: b}, nil + return multiaddr{bytes: b}, nil } // NewMultiaddrBytes initializes a Multiaddr from a byte representation. @@ -41,34 +41,33 @@ func NewMultiaddrBytes(b []byte) (a Multiaddr, err error) { return nil, err } - return &multiaddr{bytes: b}, nil + return multiaddr{bytes: b}, nil } // Equal tests whether two multiaddrs are equal -func (m *multiaddr) Equal(m2 Multiaddr) bool { +func (m multiaddr) Equal(m2 Multiaddr) bool { return bytes.Equal(m.bytes, m2.Bytes()) } // Bytes returns the []byte representation of this Multiaddr -func (m *multiaddr) Bytes() []byte { - // consider returning copy to prevent changing underneath us? - cpy := make([]byte, len(m.bytes)) - copy(cpy, m.bytes) - return cpy +// +// Do not modify the returned buffer, it may be shared. +func (m multiaddr) Bytes() []byte { + return m.bytes } // String returns the string representation of a Multiaddr -func (m *multiaddr) String() string { +func (m multiaddr) String() string { s, err := bytesToString(m.bytes) if err != nil { - panic("multiaddr failed to convert back to string. corrupted?") + panic(fmt.Errorf("multiaddr failed to convert back to string. corrupted? %s", err)) } return s } // Protocols returns the list of protocols this Multiaddr has. // will panic in case we access bytes incorrectly. -func (m *multiaddr) Protocols() []Protocol { +func (m multiaddr) Protocols() []Protocol { ps := make([]Protocol, 0, 8) b := m.bytes for len(b) > 0 { @@ -97,18 +96,18 @@ func (m *multiaddr) Protocols() []Protocol { } // Encapsulate wraps a given Multiaddr, returning the resulting joined Multiaddr -func (m *multiaddr) Encapsulate(o Multiaddr) Multiaddr { +func (m multiaddr) Encapsulate(o Multiaddr) Multiaddr { mb := m.bytes ob := o.Bytes() b := make([]byte, len(mb)+len(ob)) copy(b, mb) copy(b[len(mb):], ob) - return &multiaddr{bytes: b} + return multiaddr{bytes: b} } // Decapsulate unwraps Multiaddr up until the given Multiaddr is found. -func (m *multiaddr) Decapsulate(o Multiaddr) Multiaddr { +func (m multiaddr) Decapsulate(o Multiaddr) Multiaddr { s1 := m.String() s2 := o.String() i := strings.LastIndex(s1, s2) @@ -116,7 +115,7 @@ func (m *multiaddr) Decapsulate(o Multiaddr) Multiaddr { // if multiaddr not contained, returns a copy. cpy := make([]byte, len(m.bytes)) copy(cpy, m.bytes) - return &multiaddr{bytes: cpy} + return multiaddr{bytes: cpy} } ma, err := NewMultiaddr(s1[:i]) @@ -128,7 +127,7 @@ func (m *multiaddr) Decapsulate(o Multiaddr) Multiaddr { var ErrProtocolNotFound = fmt.Errorf("protocol not found in multiaddr") -func (m *multiaddr) ValueForProtocol(code int) (string, error) { +func (m multiaddr) ValueForProtocol(code int) (string, error) { for _, sub := range Split(m) { p := sub.Protocols()[0] if p.Code == code { diff --git a/multiaddr_test.go b/multiaddr_test.go index 8c64a79..09fd343 100644 --- a/multiaddr_test.go +++ b/multiaddr_test.go @@ -229,12 +229,6 @@ func TestBytesSplitAndJoin(t *testing.T) { t.Errorf("joined components failed: %s != %s", m, joined) } - // modifying underlying bytes is fine. - m2 := m.(*multiaddr) - for i := range m2.bytes { - m2.bytes[i] = 0 - } - for i, a := range split { if a.String() != res[i] { t.Errorf("split component failed: %s != %s", a, res[i]) diff --git a/util.go b/util.go index d1b54af..e3a2913 100644 --- a/util.go +++ b/util.go @@ -11,7 +11,7 @@ func Split(m Multiaddr) []Multiaddr { addrs := make([]Multiaddr, len(split)) for i, addr := range split { - addrs[i] = &multiaddr{bytes: addr} + addrs[i] = multiaddr{bytes: addr} } return addrs } @@ -34,7 +34,7 @@ func Join(ms ...Multiaddr) Multiaddr { bidx++ } } - return &multiaddr{bytes: b} + return multiaddr{bytes: b} } // Cast re-casts a byte slice as a multiaddr. will panic if it fails to parse. @@ -43,7 +43,7 @@ func Cast(b []byte) Multiaddr { if err != nil { panic(fmt.Errorf("multiaddr failed to parse: %s", err)) } - return &multiaddr{bytes: b} + return multiaddr{bytes: b} } // StringCast like Cast, but parses a string. Will also panic if it fails to parse. From 0f158f116351b51a5402936b366885f1772e3c91 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 1 Oct 2018 15:41:44 -0700 Subject: [PATCH 2/6] nit: mark test helper --- multiaddr_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/multiaddr_test.go b/multiaddr_test.go index 09fd343..42c963d 100644 --- a/multiaddr_test.go +++ b/multiaddr_test.go @@ -179,6 +179,7 @@ func TestStringToBytes(t *testing.T) { func TestBytesToString(t *testing.T) { testString := func(s1 string, h string) { + t.Helper() b, err := hex.DecodeString(h) if err != nil { t.Error("failed to decode hex", h) From 2999d4efba8e178c2cd7e146b58f4441eddc33bc Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 1 Oct 2018 15:44:27 -0700 Subject: [PATCH 3/6] small Join optimizations --- util.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/util.go b/util.go index e3a2913..5753628 100644 --- a/util.go +++ b/util.go @@ -18,6 +18,14 @@ func Split(m Multiaddr) []Multiaddr { // Join returns a combination of addresses. func Join(ms ...Multiaddr) Multiaddr { + switch len(ms) { + case 0: + // empty multiaddr, unfortunately, we have callers that rely on + // this contract. + return multiaddr{} + case 1: + return ms[0] + } length := 0 bs := make([][]byte, len(ms)) @@ -29,10 +37,7 @@ func Join(ms ...Multiaddr) Multiaddr { bidx := 0 b := make([]byte, length) for _, mb := range bs { - for i := range mb { - b[bidx] = mb[i] - bidx++ - } + bidx += copy(b[bidx:], mb) } return multiaddr{bytes: b} } From 6c70a3d7b53a0d203c2e9887cbf2abd627bb7c11 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 1 Oct 2018 15:45:43 -0700 Subject: [PATCH 4/6] remove protocol correctness test This is checked when the protocol is *registered*. There's no reason to test it twice. --- codec.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/codec.go b/codec.go index 1f912f8..9c0c5bc 100644 --- a/codec.go +++ b/codec.go @@ -43,9 +43,6 @@ func stringToBytes(s string) ([]byte, error) { sp = []string{"/" + strings.Join(sp, "/")} } - if p.Transcoder == nil { - return nil, fmt.Errorf("no transcoder for %s protocol", p.Name) - } a, err := p.Transcoder.StringToBytes(sp[0]) if err != nil { return nil, fmt.Errorf("failed to parse %s: %s %s", p.Name, sp[0], err) From 2e9c819c180849f2ca5c21096ba953daafd5e730 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 1 Oct 2018 15:47:59 -0700 Subject: [PATCH 5/6] make cast use NewMultiaddrBytes This will perform all the correct checks *except* that it won't bother actually stringifying the multiaddr. It should be significantly faster. --- util.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/util.go b/util.go index 5753628..49eff9d 100644 --- a/util.go +++ b/util.go @@ -44,11 +44,11 @@ func Join(ms ...Multiaddr) Multiaddr { // Cast re-casts a byte slice as a multiaddr. will panic if it fails to parse. func Cast(b []byte) Multiaddr { - _, err := bytesToString(b) + m, err := NewMultiaddrBytes(b) if err != nil { panic(fmt.Errorf("multiaddr failed to parse: %s", err)) } - return multiaddr{bytes: b} + return m } // StringCast like Cast, but parses a string. Will also panic if it fails to parse. From 7989a080afa7d14c32f27fc087f0a65a7d620db2 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 1 Oct 2018 17:10:03 -0700 Subject: [PATCH 6/6] don't put the buffer struct on the heap This showed up when profile go-ipfs. --- codec.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codec.go b/codec.go index 9c0c5bc..fe672ac 100644 --- a/codec.go +++ b/codec.go @@ -11,7 +11,7 @@ func stringToBytes(s string) ([]byte, error) { // consume trailing slashes s = strings.TrimRight(s, "/") - b := new(bytes.Buffer) + var b bytes.Buffer sp := strings.Split(s, "/") if sp[0] != "" {