use string instead of struct backing #47

dignifiedquire · 2018-05-17T14:44:50Z

We call String all over the place so we should make sure it remains fast.

Stebalien · 2018-05-17T15:42:46Z

I like this. However, it's a massively breaking change so we'll have to be careful (I've received a lot of push-back from @whyrusleeping when trying to do this kind of thing in the past). Also, this makes extracting a multihash from a CID allocate (could be fixed by using a string internally) and means that constructing a CID involves copying bytes. We can fix the former by storing multihashes as strings (multiformats/go-multihash#29) and the latter by adding a function to multihash that writes to a writer (e.g., a string builder) which we'll probably want anyways.

A less breaking-change version would be to use a pointer to a string (or a pointer to, e.g., struct Cid{ string }). However, that will require an additional allocation (although the KeyString method becomes trivial which is my personal goal).

Performance wise, implementing the varint skipping code manually may be faster. That is, to skip two varints, count two bytes with the high-order bit unset.

Regardless, I think we should wait for the libp2p refactor + dht fixes + log fixes + everything else that is currently blocked.

Personally, I'd like to just break things and fix this all at once but this really is a massively breaking change. All cid == nil checks now no longer work.

whyrusleeping · 2018-05-29T06:52:26Z

@dignifiedquire please familiarize yourself with the other attempts to do similar things, primarily: multiformats/go-multiaddr#62

dignifiedquire · 2018-05-29T09:06:09Z

@whyrusleeping I did read through that discussion, but didn't see that there was a final decision made on the direction to go, so this is based on a conversation I had with @Stebalien afterwards.

I am happy to adjust if you and @Stebalien get to an agreement on which version exactly you want to go for in these, just let me know.

kevina · 2018-06-15T20:57:43Z

cid.go

+// - hash mh.Multihash
+type Cid string
+
+var EmptyCid = Cid(string([]byte{}))


Why string([]byte{}) and not just ""?

I would also make this a constant so we have:

const EmptyCid = Cid("")

Also. I find EmptyCid a bit tedious of a name. How out just Nil"? When used outside this package it would be cid.Nil.

kevina

Most of my comments are just suggestions but I am blocking for two reasons (1) The KeyString() method is wrong and needs to be fixed. (2) I opened a second p.r. (#71) where I implemented my suggestions.

kevina · 2018-08-25T06:54:35Z

cid.go

-func (c *Cid) KeyString() string {
-	return string(c.Bytes())
+func (c Cid) KeyString() string {
+	return string(c)


This is wrong. If the version is 0 only the Multihash should be returned.

This should actually be right. If the version is 0, this will only be the multihash.

Ah. Nvm, missed #47 (comment)

kevina · 2018-08-25T06:55:06Z

cid.go

+
+var EmptyCid = Cid(string([]byte{}))
+
+func (c Cid) version() uint64 {


We should export this method, it can be useful.

kevina · 2018-08-25T06:58:15Z

cid.go

-		hash:    mhash,
-	}
+func NewCidV0(mhash mh.Multihash) Cid {
+	return newCid(0, DagProtobuf, mhash)


I find this representation of CidV0 odd. Why not just use the normal binary representation, i.e, just the multihash?

kevina · 2018-08-25T07:01:12Z

cid.go

+// - hash mh.Multihash
+type Cid string
+
+var EmptyCid = Cid(string([]byte{}))


Also. I find EmptyCid a bit tedious of a name. How out just Nil"? When used outside this package it would be cid.Nil.

kevina · 2018-08-25T07:21:25Z

cid.go

-		c.version == o.version &&
-		bytes.Equal(c.hash, o.hash)
+func (c Cid) Equals(o Cid) bool {
+	// TODO: can we use regular string equality?


can we use regular string equality?

Yes.

kevina · 2018-08-25T07:24:25Z

cid.go

+// - version uvarint
+// - codec uvarint
+// - hash mh.Multihash
+type Cid string


Maybe we should make this type Cid struct{ string } to ensure the Cid type is always valid.

kevina · 2018-08-25T07:40:17Z

cid_test.go

+	}
+}
+
+// making sure we don't allocate when returning bytes


But we do now and can't avoid it. I take it BenchmarkBytesV1 is leftover from the first attempt with used []byte as the representation.

Stebalien · 2018-08-25T18:26:40Z

@dignifiedquire for context, this is moving forward due to:

#70, the fact that by-value CIDs will work better with refmt, and the fact that we're reworking a ton of CID stuff anyways for the base32 endeavor.

dignifiedquire · 2018-08-25T18:57:28Z

@Stebalien thanks for the heads up, I was following the other thread. Let me know if you need anything from me, otherwise feel free to take the code and use what you can, and throw away what you don’t like :)

kevina · 2018-08-31T05:17:21Z

Closing in favor of #71

Stebalien and others added 4 commits April 20, 2018 10:30

add String benchmark

93b08a6

We call String all over the place so we should make sure it remains fast.

feat: use CIDs as their byte representation instead of a struct

2639b67

use string instead of []byte as underlying store

760ab82

fix KeyString()

7e417ec

ghost assigned dignifiedquire May 17, 2018

ghost added the status/in-progress In progress label May 17, 2018

dignifiedquire mentioned this pull request May 17, 2018

What about backing cid.Set by a sorted slice rather than a map? #45

Open

kevina reviewed Jun 15, 2018

View reviewed changes

kevina mentioned this pull request Aug 4, 2018

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

Closed

warpfork mentioned this pull request Aug 24, 2018

cid implementation roundup #70

Merged

kevina mentioned this pull request Aug 25, 2018

continuation of use string instead of struct backing #71

Merged

kevina suggested changes Aug 25, 2018

View reviewed changes

kevina mentioned this pull request Aug 29, 2018

Update to use the new string representation of a Multihash. #73

Closed

kevina closed this Aug 31, 2018

ghost removed the status/in-progress In progress label Aug 31, 2018

kevina deleted the feat/bytes branch August 31, 2018 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use string instead of struct backing #47

use string instead of struct backing #47

dignifiedquire commented May 17, 2018 •

edited

Loading

Stebalien commented May 17, 2018

whyrusleeping commented May 29, 2018

dignifiedquire commented May 29, 2018

kevina Jun 15, 2018 •

edited

Loading

kevina Jun 15, 2018

kevina Aug 25, 2018 •

edited

Loading

kevina left a comment •

edited

Loading

kevina Aug 25, 2018

Stebalien Aug 25, 2018

Stebalien Aug 25, 2018

kevina Aug 25, 2018

kevina Aug 25, 2018

kevina Aug 25, 2018 •

edited

Loading

kevina Aug 25, 2018

kevina Aug 25, 2018

kevina Aug 25, 2018

Stebalien commented Aug 25, 2018 •

edited

Loading

dignifiedquire commented Aug 25, 2018

kevina commented Aug 31, 2018


		var EmptyCid = Cid(string([]byte{}))

		func (c Cid) version() uint64 {

use string instead of struct backing #47

use string instead of struct backing #47

Conversation

dignifiedquire commented May 17, 2018 • edited Loading

Stebalien commented May 17, 2018

whyrusleeping commented May 29, 2018

dignifiedquire commented May 29, 2018

kevina Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

kevina left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Aug 25, 2018 • edited Loading

dignifiedquire commented Aug 25, 2018

kevina commented Aug 31, 2018

dignifiedquire commented May 17, 2018 •

edited

Loading

kevina Jun 15, 2018 •

edited

Loading

kevina Aug 25, 2018 •

edited

Loading

kevina left a comment •

edited

Loading

kevina Aug 25, 2018 •

edited

Loading

Stebalien commented Aug 25, 2018 •

edited

Loading