Skip to content

Commit

Permalink
perf: outline logic in Decode to allow for stack allocations
Browse files Browse the repository at this point in the history
I took extra efforts for this to be a backward compatible change, I think `DecodedMultihash` should return a value struct not a pointer.

I also updated the error type to a value because this allows for 1 instead of 2 allocations when erroring.

```
name       old time/op    new time/op    delta
Decode-12     102ns ± 3%      18ns ± 3%   -82.47%  (p=0.000 n=9+9)

name       old alloc/op   new alloc/op   delta
Decode-12     64.0B ± 0%      0.0B       -100.00%  (p=0.000 n=10+10)

name       old allocs/op  new allocs/op  delta
Decode-12      1.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)
```

I originally found this problem by benchmarking `go-cid`:
```
github.com/ipfs/go-cid.CidFromBytes

/home/hugo/go/pkg/mod/github.com/ipfs/go-cid@v0.4.0/cid.go

  Total:      4.64GB    10.75GB (flat, cum)   100%
    638            .          .           	if len(data) > 2 && data[0] == mh.SHA2_256 && data[1] == 32 {
    639            .          .           		if len(data) < 34 {
    640            .          .           			return 0, Undef, ErrInvalidCid{fmt.Errorf("not enough bytes for cid v0")}
    641            .          .           		}
    642            .          .
    643            .     6.11GB           		h, err := mh.Cast(data[:34])
                                                               _, err := Decode(buf)                                        multihash.go:215

    644            .          .           		if err != nil {
    645            .          .           			return 0, Undef, ErrInvalidCid{err}
    646            .          .           		}
```

We can see it call `mh.Cast` and `mh.Cast` call `Decode` and instantly drops the `DecodedMultihash`.
The point of this is purely to validate the multihash by checking err.
  • Loading branch information
Jorropo committed Jun 12, 2023
1 parent d2f43bc commit e5d5390
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 29 deletions.
11 changes: 11 additions & 0 deletions allocate_go119_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//go:build !go1.20

package multihash

import "testing"

func mustNotAllocateMore(_ *testing.T, _ float64, f func()) {
// the compiler isn't able to detect our outlined stack allocation on before
// 1.20 so let's not test for it. We don't mind if outdated versions are slightly slower.
f()
}
12 changes: 12 additions & 0 deletions allocate_go120_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//go:build go1.20

package multihash

import "testing"

func mustNotAllocateMore(t *testing.T, n float64, f func()) {
t.Helper()
if b := testing.AllocsPerRun(10, f); b > n {
t.Errorf("it allocated %f max %f !", b, n)
}
}
22 changes: 18 additions & 4 deletions multihash.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var (

// ErrInconsistentLen is returned when a decoded multihash has an inconsistent length
type ErrInconsistentLen struct {
dm *DecodedMultihash
dm DecodedMultihash
lengthFound int
}

Expand Down Expand Up @@ -222,20 +222,34 @@ func Cast(buf []byte) (Multihash, error) {

// Decode parses multihash bytes into a DecodedMultihash.
func Decode(buf []byte) (*DecodedMultihash, error) {
rlen, code, hdig, err := readMultihashFromBuf(buf)
// outline decode allowing the &dm expression to be inlined into the caller.
// This moves the heap allocation into the caller and if the caller doesn't
// leak dm the compiler will use a stack allocation instead.
// If you do not outline this &dm always heap allocate since the pointer is
// returned which cause a heap allocation because Decode's stack frame is
// about to disapear.
dm, err := decode(buf)
if err != nil {
return nil, err
}
return &dm, nil
}

func decode(buf []byte) (dm DecodedMultihash, err error) {
rlen, code, hdig, err := readMultihashFromBuf(buf)
if err != nil {
return DecodedMultihash{}, err
}

dm := &DecodedMultihash{
dm = DecodedMultihash{
Code: code,
Name: Codes[code],
Length: len(hdig),
Digest: hdig,
}

if len(buf) != rlen {
return nil, ErrInconsistentLen{dm, rlen}
return dm, ErrInconsistentLen{dm, rlen}
}

return dm, nil
Expand Down
78 changes: 53 additions & 25 deletions multihash_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,27 +151,29 @@ func TestDecode(t *testing.T) {

nb := append(pre[:n], ob...)

dec, err := Decode(nb)
if err != nil {
t.Error(err)
continue
}
mustNotAllocateMore(t, 0, func() {
dec, err := Decode(nb)
if err != nil {
t.Error(err)
return
}

if dec.Code != tc.code {
t.Error("decoded code mismatch: ", dec.Code, tc.code)
}
if dec.Code != tc.code {
t.Error("decoded code mismatch: ", dec.Code, tc.code)
}

if dec.Name != tc.name {
t.Error("decoded name mismatch: ", dec.Name, tc.name)
}
if dec.Name != tc.name {
t.Error("decoded name mismatch: ", dec.Name, tc.name)
}

if dec.Length != len(ob) {
t.Error("decoded length mismatch: ", dec.Length, len(ob))
}
if dec.Length != len(ob) {
t.Error("decoded length mismatch: ", dec.Length, len(ob))
}

if !bytes.Equal(dec.Digest, ob) {
t.Error("decoded byte mismatch: ", dec.Digest, ob)
}
if !bytes.Equal(dec.Digest, ob) {
t.Error("decoded byte mismatch: ", dec.Digest, ob)
}
})
}
}

Expand Down Expand Up @@ -242,15 +244,20 @@ func TestCast(t *testing.T) {

nb := append(pre[:n], ob...)

if _, err := Cast(nb); err != nil {
t.Error(err)
continue
}
mustNotAllocateMore(t, 0, func() {
if _, err := Cast(nb); err != nil {
t.Error(err)
return
}
})

if _, err = Cast(ob); err == nil {
t.Error("cast failed to detect non-multihash")
continue
}
// 1 for the error object.
mustNotAllocateMore(t, 1, func() {
if _, err = Cast(ob); err == nil {
t.Error("cast failed to detect non-multihash")
return
}
})
}
}

Expand Down Expand Up @@ -343,8 +350,29 @@ func BenchmarkDecode(b *testing.B) {
pre[1] = byte(uint8(len(ob)))
nb := append(pre, ob...)

b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
Decode(nb)
}
}

func BenchmarkCast(b *testing.B) {
tc := testCases[0]
ob, err := hex.DecodeString(tc.hex)
if err != nil {
b.Error(err)
return
}

pre := make([]byte, 2)
pre[0] = byte(uint8(tc.code))
pre[1] = byte(uint8(len(ob)))
nb := append(pre, ob...)

b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
Cast(nb)
}
}

0 comments on commit e5d5390

Please sign in to comment.