-
Notifications
You must be signed in to change notification settings - Fork 47
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
Benchmark existing ways to check for IDENTITY
CIDs
#135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop the CidV1
part of the benchmark name; Cid is redundant since this module is go-cid, and v1 is redundant as we don't really care if the CID is v1 or not - it just so happens that all identity CIDs are currently v1. If they also supported a future CIDv2, then the benchmark would simply do both in each iteration.
You could also make the two methods sub-benchmarks, to simplify sharing code and make the output nicer. For example:
func BenchmarkIdentityCheck(b *testing.B) {
// cid setup, ReportAllocs, etc
b.Run("Prefix", func(b *testing.B) {
b.RunParallel(func(pb *testing.PB) {
// check via Prefix
})
})
b.Run("MultihashDecode", func(b *testing.B) {
b.RunParallel(func(pb *testing.PB) {
// check via MultihashDecode
})
})
}
eb5e77d
to
ada55fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM! Probably wait for a review from a steward though.
ada55fd
to
c2a6e2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no reason to complain about more benchmarks :)
If you still have the context in your head: I wouldn't mind a short paragraph of comment attached to the function about what you were looking for / why it mattered / and what the rough findings were at the time you committed. (It'll get stale, sure -- but that doesn't mean it won't still be useful context clues to some future archaeologist^W reviewer.)
(... I guess that would be roughly the same content you've already included in the PR message. Still worth repeating in the diff, IMO. :) I find it a lot easier to rediscover links in a comment in the relevant code than... links that are in PRs that are maybe (depending on which merge button you click in github) mentioned by number in the commit log that you might find after you go blame-splunking.)
2a5f393
to
44b768d
Compare
Add benchmarks that compare two ways of checking for `multihash.IDENTITY` code: 1. `Cid.Prefix().MhType` 2. Decode of `Cid.Hash()` This benchmark illustrates that using Cid.Prefix is more efficient than `multihash.Decode`. Users wishing to perform such a check should use `Cid.Prefix`. Consider that `Cid.Prefix` is already efficient enough and introducing a dedicated API for performing this check will likely result in small gains. Relates to #133
44b768d
to
70c228c
Compare
Benchmark existing ways to check for
IDENTITY
CIDsAdd benchmarks that compare two ways of checking for
multihash.IDENTITY
code:Cid.Prefix().MhType
Cid.Hash()
This benchmark illustrates that using Cid.Prefix is more efficient than
multihash.Decode
. Users wishing to perform such a check should useCid.Prefix
.Consider that
Cid.Prefix
is already efficient enough and introducing adedicated API for performing this check will likely result in small
gains.
Relates to #133