search

ipfs / go-bitswap

The golang implementation of the bitswap protocol
MIT License
216 stars 112 forks source link

fix: shard lock to fix contention issue in the notify handler #561

Closed Jorropo closed 1 year ago

Jorropo commented 2 years ago

I have many profiles where I had 5k+ goroutines all waiting on connectEventManager.lk while downloading. Capture d’écran du 2022-04-06 04-07-49

Turns out it defer the unlock call, and call to an other function, however all of the other sub handlers all have their own locking mechanism. So I've made it unlock first then call to the next function. (this breaks order as maybe a handler had side effect that relied on peers being handled in the same order as an other one but AFAIK this isn't a thing, all the sub handlers are fairly sane doesn't have such side effect).

I've done the same fix in engine because why not ? (as the exact same argument of consumers locking too can be made) and this completely removed it from the profile.

This patch completely removes those functions from the profile for me.

Jorropo commented 2 years ago

NVM tests are failing ...

Stebalien commented 2 years ago

Now that #565 has landed, we should look at this again. Before #565, this wouldn't have worked because we would have been able to get out-of-order notifications. However, now that we have a central event loop, we can probably get away with this?

Jorropo commented 1 year ago

This repository has been moved to https://github.com/ipfs/go-libipfs. There is not an easy way to transfer PRs, so if you would like to continue with this PR then please re-open it in the new repository and link to this PR.