Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added semaphore implementation with lower contention #298

Merged
merged 1 commit into from
Jun 24, 2020

Conversation

merlimat
Copy link
Contributor

Motivation

When there are multiple go-routines sharing a single producer instance, there can be significant contention on the channel that is used as a semaphore.

That is because the channel is internally implemented with a mutex.

Instead, use a counter to optimize for the normal case scenario (when we're not blocked by the semaphore), leaving the channel as a way to synchronize when the semaphore has exhausted all the permits.

@addisonj
Copy link
Contributor

This looks like a nice improvement, one question I have though: is this now perhaps sane to have it be backed by https://godoc.org/golang.org/x/sync/semaphore? It seems like you could perhaps have most of the details covered with that package and add the details for blocked threads waiting for permits as part of a small wrapper? I am not 100% sure about the semaphore packages semantics to know if that would work but just what came to mind as I looked at the difference between the packages.

@aahmed-se
Copy link

@addisonj golang semaphore are channel based which are not as fast as this implementation.

@aahmed-se
Copy link

@merlimat have you looked into this library it's benchmarked against the other semaphore implementations.
https://github.com/marusama/semaphore

@merlimat
Copy link
Contributor Author

@addisonj The x/sync/semaphore has more features (tryAcquire, weighted) but it's still using mutex, even in best case so it has the same contention profile.

@aahmed-se I've checked that lib and it has the same approach used here though it has more features which we don't need (change limit, weighted). For these additional features it needs to have the RW-mutex and change the channel each time.
Since we don't use that, we should avoid paying for it.

@merlimat merlimat merged commit cc08fd6 into apache:master Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants