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

fastpath CBOR #64

Merged
merged 1 commit into from
Sep 17, 2019
Merged

fastpath CBOR #64

merged 1 commit into from
Sep 17, 2019

Conversation

tobowers
Copy link
Contributor

@tobowers tobowers commented Sep 2, 2019

I realize this might not be mergable as is... because it changes how Tree() and Links() are handled and doesn't error in the same place as before.

I was looking at this PR: https://github.com/ipfs/go-hamt-ipld/pull/29/files and looking at some benchmarks noticed that refmt was still taking up a bunch of time in our blockservice interface and I realized that's because when the block comes back it is still decoded into an interface{}.

This PR allows lazy loading of Tree and Links (which are often unused) and allows fast-path cbor if objects support them.

@tobowers
Copy link
Contributor Author

tobowers commented Sep 4, 2019

Thoughts here? especially @whyrusleeping

refmt.go Outdated Show resolved Hide resolved
@tobowers
Copy link
Contributor Author

Hiyo! Does this have any chance of getting merged? Looking for feedback here.

@hannahhoward
Copy link

@tobowers @warpfork I wonder if we can just merge the changes to marshaller and unmarshaller -- I could use them over in Filecoin land.

encoding/cloner.go Outdated Show resolved Hide resolved
var err error
selfMarshaling, ok := obj.(cborMarshaler)
if ok {
err = selfMarshaling.MarshalCBOR(w)
Copy link
Member

Choose a reason for hiding this comment

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

+1 this is what i had in mind

m.reader.r = r
err := m.unmarshal.Unmarshal(obj)
selfUnmarshaler, ok := obj.(cborUnmarshaler)
Copy link
Member

Choose a reason for hiding this comment

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

+1

node.go Outdated
@@ -261,6 +274,10 @@ func (n *Node) ResolveLink(path []string) (*node.Link, []string, error) {

// Tree returns a flattend array of paths at the given path for the given depth.
func (n *Node) Tree(path string, depth int) []string {
err := n.setTreeAndLinks()
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

quietly dropping the error here makes me uncomfortable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but I didn't want to change the interface.

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

The marshaling and cloner changes LGTM. Happy to ship those right now.

The changes to the tree and links thing is another story though. Definitely not comfortable ignoring errors.

I've also been thinking about how to improve this separately, and I think that writing a piece of code that just iterates through the raw cbor directly to pull out the links and tree. This can be way faster than the current way we do it (should be as fast, if not faster than, the parsing itself, and could potentially be combined with the parsing in a future optimization). If that sounds good to you, i'd say we pull those changes out of this PR for now, get this merged, and then do that as a followup.

@whyrusleeping
Copy link
Member

(also, sorry for the slowness in response, been not checking my email lately)

@tobowers
Copy link
Contributor Author

ok, I made the incremental change to support objects which can encode/decode themselves. That being said, this repo won't see any speed improvement due to these changes until we can fix the decode into interface{}

@tobowers
Copy link
Contributor Author

Oh... I guess I'm wrong... only projects using merkledag.Decode will still have the slow path... but general BlockService and Blockstore will not actually go through that method.

Copy link

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

Not one of designated reviewers, but this now looks LGTM to me. @warpfork / @whyrusleeping ?

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

LGTM, let's get this in

@whyrusleeping whyrusleeping merged commit 42a6b22 into ipfs:master Sep 17, 2019
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.

4 participants