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

Implement efficient function to check for IDENTITY multihash code #134

Closed
wants to merge 1 commit into from

Conversation

masih
Copy link
Member

@masih masih commented Sep 24, 2021

Implement a fail-fast function that checks whether the code of a CID
is multihash.IDENTITY or not.

Add benchmarks that compare three ways of checking for
multihash.IDENTITY code:

  1. Cid.Prefix().MhType
  2. Decode of Cid.Has()
  3. The new Cid.IsIdentity() API
BenchmarkCidV1IdentityCheckUsingPrefix
BenchmarkCidV1IdentityCheckUsingPrefix-8            	355396287	         3.334 ns/op	332899.32 MB/s	       0 B/op	       0 allocs/op
BenchmarkCidV1IdentityCheckUsingMultihashDecode
BenchmarkCidV1IdentityCheckUsingMultihashDecode-8   	 6404062	       194.2 ns/op	5715.67 MB/s	    1216 B/op	       2 allocs/op
BenchmarkCidV1IdentityCheckUsingIsIdentity
BenchmarkCidV1IdentityCheckUsingIsIdentity-8        	442939464	         2.621 ns/op	423535.07 MB/s	       0 B/op	       0 allocs/op

Fixes #133

Implement a fail-fast function that checks whether the code of a CID
is `multihash.IDENTITY` or not.

Add benchmarks that compare three ways of checking for
`multihash.IDENTITY` code:
1. `Cid.Prefix().MhType`
2. Decode of `Cid.Has()`
3. The new `Cid.IsIdentity()` API

Fixes #133
@masih masih requested a review from mvdan September 24, 2021 21:11

var n int
// Skip version.
_, n, err := uvarint(c.str[n:])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_, n, err := uvarint(c.str[n:])
_, n, err := uvarint(c.str)

return false, nil
}

var n int
Copy link
Member

Choose a reason for hiding this comment

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

No need to declare n here.

if err != nil {
return false, err
}
n += n
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean do declare a bytesRead int, and add n every time you read a varint? Now you're just overwriting n every time.

@Stebalien
Copy link
Member

We really shouldn't be special-casing "identity" CIDs at this layer. If we really need this for performance, we should have a function to get the multihash codec.

But I'm not convinced it matters. We're saving less than a nanosecond per op, which'll be nothing compared to all the other work we're going to end up doing.

@masih
Copy link
Member Author

masih commented Sep 27, 2021

Thank you @marten-seemann and @Stebalien for the reviews 🍻
I agree with @Stebalien.

Closing this as it does not add much value.

@masih
Copy link
Member Author

masih commented Sep 27, 2021

@marten-seemann @Stebalien I have extracted benchmarks from this PR into a new PR as a way to document the efficiency of existing APIs for IDENTITY check. I think that's worth having 🙂

See: #135

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.

Provide an efficient API to check whether a CID has IDENTITY multihash code
3 participants