Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove *Serial types. Use pointers for all types. #688

Merged
merged 8 commits into from
Mar 1, 2019

Conversation

hsanjuan
Copy link
Collaborator

This takes advantange of the latest features in go-cid, peer.ID and
go-multiaddr and makes the Go types serializable by default.

This means we no longer need to copy between Pin <-> PinSerial, or ID <->
IDSerial etc. We can now efficiently binary-encode these types using short
field keys and without parsing/stringifying (in many cases it just a cast).

We still get the same json output as before (with minor modifications for
Cids).

This should greatly improve Cluster performance and memory usage when dealing
with large collections of items.

License: MIT
Signed-off-by: Hector Sanjuan hector@protocol.ai

This takes advantange of the latest features in go-cid, peer.ID and
go-multiaddr and makes the Go types serializable by default.

This means we no longer need to copy between Pin <-> PinSerial, or ID <->
IDSerial etc. We can now efficiently binary-encode these types using short
field keys and without parsing/stringifying (in many cases it just a cast).

We still get the same json output as before (with minor modifications for
Cids).

This should greatly improve Cluster performance and memory usage when dealing
with large collections of items.

License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@ghost ghost assigned hsanjuan Feb 27, 2019
@ghost ghost added the status/in-progress In progress label Feb 27, 2019
@hsanjuan
Copy link
Collaborator Author

hsanjuan commented Feb 27, 2019

It has been HORRIBLE to do this, but I think we had to do it sooner or later. I'm super glad we have so many tests. A few things here are WIP.

The main things here are in types.go and rpc_api.go. Everything else is mostly making things work with those changes.

makeGet(t, rest, url(rest)+"/id", &id)
if id.ID != test.TestPeerID1.Pretty() {
if id.ID.Pretty() != test.TestPeerID1.Pretty() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we could do id.ID == test.TestPeerID1 but that's depending on the fact that peer IDs are strings, which may change. Ideailly, IDs should provide an Equals method, like Cids do.

ifaces := make([]interface{}, len(in), len(in))
for i := range in {
ifaces[i] = &in[i]
in[i] = &api.ID{}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not doing this causes very bad errors

License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@hsanjuan
Copy link
Collaborator Author

Since now we can work with Cids directly, I have gone and removed all the cid.Decode or MustDecodeCid() from tests. Then also renamed TestCid* to Cid* and so on.

License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@hsanjuan hsanjuan requested a review from lanzafame February 27, 2019 21:55
@hsanjuan hsanjuan changed the title [WIP] Remove *Serial types. Use pointers for all types. Remove *Serial types. Use pointers for all types. Feb 27, 2019
@hsanjuan
Copy link
Collaborator Author

Fixes #654

This was referenced Feb 27, 2019
Copy link
Contributor

@lanzafame lanzafame left a comment

Choose a reason for hiding this comment

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

Other than the debug init, LGTM.

License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@hsanjuan hsanjuan merged commit 121660a into master Mar 1, 2019
@ghost ghost removed the status/in-progress In progress label Mar 1, 2019
@hsanjuan hsanjuan deleted the feat/remove-serial branch March 1, 2019 14:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants