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

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

Closed
wants to merge 1 commit into from

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Apr 6, 2022

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
Copy link
Contributor Author

Jorropo commented Apr 6, 2022

NVM tests are failing ...

@Jorropo Jorropo marked this pull request as draft April 6, 2022 02:31
@Stebalien
Copy link
Member

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
Copy link
Contributor Author

Jorropo commented Jan 27, 2023

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.

@Jorropo Jorropo closed this Jan 27, 2023
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