fix: reduce receive contention #536
Conversation
This means we need to frequently re-take this lock, but it also means we don't hold it while calling other functions that might block (e.g., while pushing jobs).
| e.lock.RLock() | ||
| peers := e.peerLedger.Peers(k) | ||
| e.lock.RUnlock() |
There was a problem hiding this comment.
Are you assuming that the amount of time to grab the ledger maps for all the peers is long enough that it's worth locking for each peer rather than grabbing all the ledgers at once and chucking them into an array?
There was a problem hiding this comment.
No, I'm just assuming that repeatedly taking and releasing the lock isn't going to be too expensive.
| e.lock.RLock() | ||
| ledger, ok := e.ledgerMap[p] | ||
| e.lock.RUnlock() |
There was a problem hiding this comment.
Just a sanity check question. Does it matter if the operations in the unlocked portion end up happening on an invalid ledger (i.e. one that is either no longer in the map)?
There was a problem hiding this comment.
Kind of. But... that's already an issue pretty much everywhere in the code so I'm ignoring it for now. Really, we need to be reference counting, but that's a larger change.
I say kind of because all this code is a bit racy (which is why we re-broadcast wants occasional).
|
This is working well for me, except now im hitting other issues that I think the lock contention was holding back |
fix: reduce receive contention This commit was moved from ipfs/go-bitswap@1ab28b8
This means we need to frequently re-take this lock, but it also means we don't hold it while calling other functions that might block (e.g., while pushing jobs).