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

WIP: Make Cid an interface and change default representation to a String #64

Closed
wants to merge 5 commits into from

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Aug 4, 2018

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 to base32 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 to cid.Cid but it should be less painful (as nil can still be used) and only needs to be done once.

@ghost ghost assigned kevina Aug 4, 2018
@ghost ghost added the status/in-progress In progress label Aug 4, 2018
@warpfork
Copy link
Member

warpfork commented Aug 5, 2018

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 runtime.convT2E -- even when the concrete implementation would be CidString. The performance of that cast itself isn't a huge issue... except because we're in pointer land, it tends to force an allocation, and that then can become pretty costly via the eventual GC pressure if its on any sort of hot path. It's possible this gets optimized out by the compiler in some cases, but I don't know much about this, and if there are any such optimizations, I'd assume they're pretty narrow.

@warpfork
Copy link
Member

warpfork commented Aug 5, 2018

@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 *Cid -> Cid migration everywhere, syntactically... and of course limit all accesses of fields to via functions, use consts for == cid.Zero checks, etc... we could actually make future refactors between a concrete type that's a string vs a concrete type of a struct syntactically non-breaking to consumes. Sort of a poor-(gc-concerned)-man's interface, without actually being an interface, if you will. Not sure if that's useful feedback to this PR in particular, but just wanted to throw that thought out there.

@kevina
Copy link
Contributor Author

kevina commented Aug 5, 2018

@warpfork The idea behind this implementation is when it matters, you can just use the CidString type directly. In most uses of a Cid the I image the extra overhead will be negligible.

@kevina kevina changed the title Make Cid an interface and change default representation to a String WIP:Make Cid an interface and change default representation to a String Aug 6, 2018
@Stebalien
Copy link
Member

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?

@kevina
Copy link
Contributor Author

kevina commented Aug 6, 2018

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.

@kevina kevina changed the title WIP:Make Cid an interface and change default representation to a String WIP: Make Cid an interface and change default representation to a String Aug 6, 2018
@kevina
Copy link
Contributor Author

kevina commented Aug 9, 2018

I will create it later today.

See ipfs/kubo#5349

@kevina
Copy link
Contributor Author

kevina commented Aug 30, 2018

Closing, as we are going with a pure string representation. Please don't delete the branch.

@kevina kevina closed this Aug 30, 2018
@ghost ghost removed the status/in-progress In progress label Aug 30, 2018
@mvdan mvdan deleted the kevina/interface branch July 1, 2021 16:13
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.

3 participants