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

fix want gauge calculation #416

Merged
merged 3 commits into from
Jun 10, 2020
Merged

fix want gauge calculation #416

merged 3 commits into from
Jun 10, 2020

Conversation

dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Jun 10, 2020

Fixes #413

A slight performance degradation but it's not too bad:

#395

$ go test ./internal/peermanager -run xyz -v -bench . -benchtime 5s
cpu.out
goos: darwin
goarch: amd64
pkg: github.com/ipfs/go-bitswap/internal/peermanager
BenchmarkPeerManager
BenchmarkPeerManager-8   	  182610	     33038 ns/op

This branch:

$ go test ./internal/peermanager -run xyz -v -bench . -benchtime 5s
goos: darwin
goarch: amd64
pkg: github.com/ipfs/go-bitswap/internal/peermanager
BenchmarkPeerManager
BenchmarkPeerManager-8   	  169858	     35684 ns/op

@dirkmc dirkmc requested a review from Stebalien June 10, 2020 20:00
@dirkmc
Copy link
Contributor Author

dirkmc commented Jun 10, 2020

The test/race failure is a red herring 🐠 - I've fixed it in the refactor branch

internal/peermanager/peerwantmanager.go Outdated Show resolved Hide resolved
internal/peermanager/peerwantmanager.go Outdated Show resolved Hide resolved
internal/peermanager/peerwantmanager.go Outdated Show resolved Hide resolved
internal/peermanager/peerwantmanager.go Show resolved Hide resolved
@Stebalien
Copy link
Member

I'm slightly concerned about performance here on cancellation here. It's probably fine but I wouldn't trust a microbenchmark unless we're testing sessions with hundreds of peers.

Alternatively, we can probably go back to a single want gauge. We added a second one to fix #333, but the real issue was that we weren't tracking the number of items we wanted, we were tracking the number of outstanding want requests sent. Simply tracking the total number of wanted items (have and otherwise) would allow os to vastly simplify all this logic.

@dirkmc
Copy link
Contributor Author

dirkmc commented Jun 10, 2020

The benchmark tests with 500 peers. Maybe we can try running on staging and see how it fares?

I'm not sure if it actually would be simpler to track the total number of wants - we'd still need to make sure we're not double counting across peers and across want-have / want-block / broadcast wants.

@Stebalien
Copy link
Member

Ah, ok. Then it's probably fine.

I'm not sure if it actually would be simpler to track the total number of wants - we'd still need to make sure we're not double counting across peers and across want-have / want-block / broadcast wants.

We'd just track, "is wanted" for each wanted CID, right? That is, it's "wanted" if len(pwm.wantPeers[c]) > 0 || pwm.broadcastWants.Has(c).

@Stebalien Stebalien merged commit f29c774 into master Jun 10, 2020
@Stebalien Stebalien deleted the fix/want-gauge branch June 10, 2020 21:20
@dirkmc
Copy link
Contributor Author

dirkmc commented Jun 10, 2020

Ah yes, you're right that would be simpler

Jorropo pushed a commit to Jorropo/go-libipfs that referenced this pull request Jan 26, 2023
fix want gauge calculation

This commit was moved from ipfs/go-bitswap@f29c774
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.

Wantlist metrics bug
2 participants