-
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
WIP: Make Cid an interface and change default representation to a String #64
Conversation
Authors: Steven Allen <[email protected]> [email protected] Kevin Atkinson <[email protected]>
Is it possible to get some rough benchmarking of how using an interface will perform compared to doing a direct typedef of string as in #47 ? I like interface flexibility, but suspect interfaces here actually always compel some amount of performance overhead, because an interface will kick things back into effectively being pointers (because an interface itself is always two words wide: one for the concrete type info, and one for the pointer to the rest of the data -- even if the concrete type is otherwise not a pointer). As far as I understand, this would probably thus still cause lots of pointer deref overhead and cause lots and lots of casts -- stuff that will show up on profiler info and call graphs as |
@Stebalien and I were also meditating a little bit on friday about typedef-string-vs-struct and had an interesting thought: so long as we make the essential |
@warpfork The idea behind this implementation is when it matters, you can just use the |
So, I'm less worried about the performance and more worried about mixing formatting/display information with actual data. Is there a meta issue for discussing the problem we're trying to solve here? |
I will create it later today. |
See ipfs/kubo#5349 |
Closing, as we are going with a pure string representation. Please don't delete the branch. |
This builds on #47 but instead makes the
Cid
type an interface. I was initially against the idea but then realized that it will help with the switch tobase32
by allowing a base to be stored with a Cid, so that when it converted back to a string the same base can be used.This creates two new types that can be used directly: (1)
CidString
a string that is the binary representation of a Cid. This type can be used directly in Maps and when the additional overhead of an interface creates performance problems. (2)CidWithBase
a Cid with a multibase associated with it. Calling String() will reencode CidV1 using that base.This will still break code as
*cid.Cid
will need to change tocid.Cid
but it should be less painful (as nil can still be used) and only needs to be done once.