Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 12, 2023

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?

multihash.go Outdated Show resolved Hide resolved
@Jorropo Jorropo force-pushed the no-alloc-decode branch 4 times, most recently from c05eb6e to 4194c21 Compare June 12, 2023 07:39
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