Skip to content
This repository was archived by the owner on Feb 1, 2023. It is now read-only.

Simulate DONT_HAVE for older peers#248

Merged
dirkmc merged 11 commits intomasterfrom
fix/dont-have-timeout
Feb 12, 2020
Merged

Simulate DONT_HAVE for older peers#248
dirkmc merged 11 commits intomasterfrom
fix/dont-have-timeout

Conversation

@dirkmc
Copy link
Copy Markdown
Contributor

@dirkmc dirkmc commented Jan 31, 2020

When sending a want-block to a peer running an older version of Bitswap, if there is a timeout then simulate receiving a DONT_HAVE from that peer for the CID.

This PR addresses one of the two cases described in #244. It does not address timeouts caused by a peer being overloaded or network issues.

Sample benchmark output:

$ go test . -run a -bench BenchmarkFetchFromOldBitswap
goos: darwin
goarch: amd64
pkg: github.com/ipfs/go-bitswap
BenchmarkFetchFromOldBitswap/3Nodes-Overlap3-OneAtATime-8         	       1	25244830917 ns/op
BenchmarkFetchFromOldBitswap/3Nodes-AllToAll-OneAtATime-8         	1000000000	         0.230 ns/op
BenchmarkFetchFromOldBitswap/3Nodes-Overlap3-AllConcurrent-8      	1000000000	         0.0285 ns/op
BenchmarkFetchFromOldBitswap/3Nodes-Overlap3-OneAtATime (1 runs / 25.24s):  25.243s: sent 25, recv 11, dups 1 / 11
BenchmarkFetchFromOldBitswap/3Nodes-AllToAll-OneAtATime (14 runs / 3.24s):  0.231s: sent 20, recv 11, dups 1 / 11
BenchmarkFetchFromOldBitswap/3Nodes-Overlap3-AllConcurrent (7 runs / 0.17s): 0.024s: sent 2, recv 6, dups 4 / 14
PASS
ok  	github.com/ipfs/go-bitswap	29.755s

@Stebalien
Copy link
Copy Markdown
Member

(assuming you don't want a full review as this is marked as a draft)

@Stebalien Stebalien mentioned this pull request Feb 5, 2020
@dirkmc dirkmc marked this pull request as ready for review February 5, 2020 19:22
@dirkmc dirkmc requested a review from Stebalien February 10, 2020 15:41
Copy link
Copy Markdown
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

I'd like to simplify the control flow before merging this. We're launching goroutines from goroutines from goroutines which is making the control flow hard to reason about.

@dirkmc
Copy link
Copy Markdown
Contributor Author

dirkmc commented Feb 11, 2020

Screen Shot 2020-02-11 at 5 43 55 PM

I wanted to test how this branch would perform when fetching from seeds that have a subset of the data (instead of both seeds having all of the data) and compare it to master and to old Bitswap (that doesn't support HAVE / DONT_HAVE messages).

These results are for one leech fetching files of various sizes from two seeds, where each seed has 2/3 of the leaf blocks in the file:

  • master-from-2master: leech (master) fetching from two seeds (master)
    Note that master now supports HAVE / DONT_HAVE messages
  • old-from-2old: leech (old) fetching from two seeds (old)
    Note that old Bitswap does not support HAVE / DONT_HAVE messages
  • sim-dont-have-from-2old: leech (sim-dont-have) fetching from two seeds (old)
    The leech (sim-dont-have) is this branch, ie Bitswap simulating DONT_HAVE with timeouts

Interestingly the results for old Bitswap are very inconsistent, if you look closely you can see orange data points (representing old-from-2old) right near the top of the graph (behind the key) for file size 32MB. I ran this simulation twice and got the same anomaly with old Bitswap for 32MB files.

Conversely this branch (which simulates DONT_HAVE with timeouts) produces more consistent results with somewhat worse performance for larger file sizes.

Copy link
Copy Markdown
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Ok, I'm mostly down to coding nits at this point. I feel like there should be some way to simplify this, but I can't think of any.

My only real remaining concern is the want, cancel, want race. It'll be fine, but if we keep doing that, we'll stall a bit because we'll keep delaying the want.

@dirkmc
Copy link
Copy Markdown
Contributor Author

dirkmc commented Feb 12, 2020

I've resolved the want-cancel-want in the way that you suggested, and resolved the nits.

Thanks for all the detailed coding suggestions and explanations ❤️
Personally I appreciate all suggestions including nits, I always feel like I learn something.

Copy link
Copy Markdown
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

One small bug.

@dirkmc dirkmc force-pushed the fix/dont-have-timeout branch from e61c8c4 to 1a16a42 Compare February 12, 2020 21:12
@dirkmc dirkmc merged commit 20be084 into master Feb 12, 2020
@dirkmc dirkmc deleted the fix/dont-have-timeout branch February 12, 2020 21:26
@Stebalien
Copy link
Copy Markdown
Member

🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants