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

Update go-graphsync v0.8.0 #212

Closed
wants to merge 2 commits into from
Closed

Conversation

hannahhoward
Copy link
Collaborator

Goals

Good news: The latest version of go-graphync and go-ipld-prime contains an important optimization to remove a bunch of unneccesary hashing when reading from local blockstores, which should cut down significantly on CPU usage
Bad news: The latest versions of these libraries contain a some important, but very breaking changes to syntax.
Good news: I'm updating them all the way to Lotus! And also, in most cases, from the point of view of the consuming libraries, they're simplifications.

Implementation

  • Update to go-graphsync v0.8.0 with go-ipld-prime linksystem branch & trusted store.
  • Fix syntax changes which are mostly to:
    • dagcbor Encode / Decode function name
    • use of linksystem over loader + storer.

Background

Update to go-graphsync v0.8.0 with go-ipld-prime linksystem branch & trusted store.
@codecov-commenter
Copy link

Codecov Report

Merging #212 (e4f40bc) into master (db84ad0) will increase coverage by 0.11%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
+ Coverage   64.84%   64.96%   +0.11%     
==========================================
  Files          24       24              
  Lines        2600     2600              
==========================================
+ Hits         1686     1689       +3     
+ Misses        573      571       -2     
+ Partials      341      340       -1     
Impacted Files Coverage Δ
message/message1_1/transfer_message_cbor_gen.go 34.52% <0.00%> (ø)
message/message1_1/transfer_request_cbor_gen.go 27.17% <0.00%> (ø)
message/message1_1/transfer_response_cbor_gen.go 29.68% <0.00%> (ø)
channels/channel_state.go 80.55% <100.00%> (ø)
encoding/encoding.go 60.00% <100.00%> (ø)
message/message1_1/transfer_request.go 76.36% <100.00%> (ø)
transport/graphsync/graphsync.go 77.48% <100.00%> (ø)
impl/receiver.go 67.10% <0.00%> (-1.32%) ⬇️
network/libp2p_impl.go 72.26% <0.00%> (+3.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db84ad0...e4f40bc. Read the comment docs.

@tchardin
Copy link
Contributor

tchardin commented Jun 11, 2021

@dirkmc what is the timing for this? Unfortunately we cannot use the latest go-data-transfer release because it relies on changes made to an older version of graphsync that is incompatible with ipld-prime v0.7.0 used in our project. Cannot run it from the commit either since there are breaking changes with go-fil-markets. So sadly we can't benefit from the changes you made until this gets in. Thanks!

@dirkmc
Copy link
Contributor

dirkmc commented Jun 11, 2021

@tchardin we've been doing some performance testing of graphsync v0.8.0. You can follow the progress at #166

It seems like there may need to be further optimizations made before a newer version of graphsync can be integrated into go-data-transfer.

I'd suggest you align your code with the latest release of go-data-transfer v1.7.0 which uses graphsync v0.6.4

@tchardin
Copy link
Contributor

Gotcha, thanks @dirkmc. So you confirm it would be faster to downgrade our project to ipld-prime v0.5.1 instead of waiting for this to get in?

@dirkmc
Copy link
Contributor

dirkmc commented Jun 11, 2021

I think that's probably your best bet 👍

@hannahhoward
Copy link
Collaborator Author

superseded by #246

@dirkmc dirkmc mentioned this pull request Oct 4, 2021
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