Skip to content

perf: outline logic in Decode to allow for stack allocations#174

Merged
Jorropo merged 2 commits intomasterfrom
no-alloc-decode
Jun 12, 2023
Merged

perf: outline logic in Decode to allow for stack allocations#174
Jorropo merged 2 commits intomasterfrom
no-alloc-decode

Conversation

@Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Jun 12, 2023

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/[email protected]/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.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test that asserts the 0 allocs are happening?

@Jorropo Jorropo force-pushed the no-alloc-decode branch 4 times, most recently from c05eb6e to 4194c21 Compare June 12, 2023 07:39
Jorropo added 2 commits June 12, 2023 09:42
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/[email protected]/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.
@github-actions
Copy link

Suggested version: v0.2.3

Comparing to: v0.2.2 (diff)

Changes in go.mod file(s):

(empty)

gorelease says:

# summary
Suggested version: v0.2.3

gocompat says:

Your branch is up to date with 'origin/master'.

Cutting a Release (and modifying non-markdown files)

This PR is modifying both version.json and non-markdown files.
The Release Checker is not able to analyse files that are not checked in to master. This might cause the above analysis to be inaccurate.
Please consider performing all the code changes in a separate PR before cutting the release.

Automatically created GitHub Release

A draft GitHub Release has been created.
It is going to be published when this PR is merged.
You can modify its' body to include any release notes you wish to include with the release.

@Jorropo Jorropo merged commit ff9da31 into master Jun 12, 2023
@Jorropo Jorropo deleted the no-alloc-decode branch June 12, 2023 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants