sync icon indicating copy to clipboard operation
sync copied to clipboard

semaphore: make semaphore resizable

Open sherifabdlnaby opened this issue 7 years ago • 7 comments

Semaphores are often used to bound concurrency (e.g worker-pool) and it is sometimes preferred to be able to resize the semaphore/workerpool to bound concurrency dynamically in respond to load.

This implementation is more performant than channel-based semaphore (channel based semaphore can't be resized as channel buffer is immutable),There is another implementations of a non-channel-based semaphores that supports resizing, but I found many bugs/deadlocks using them and they're all less performant than x/sync/semaphore.

The current implementation of x/sync/semaphore can be easily extended to be resizable, without affecting performance by any means.

Github Issue: https://github.com/golang/go/issues/29721

Edit:

I think I made a small mistake, I already posted the CR on Gerrit, I submitted this PR just for visibility and assumed from the repo's other PR(s) that the bot won't import it to Gerrit. The code is identical, so I think abandoning any of the duplicate changes is fine. Original Gerrit Review: https://go-review.googlesource.com/c/sync/+/157718

sherifabdlnaby avatar Jan 13 '19 20:01 sherifabdlnaby

This PR (HEAD: b0fff298b718af3950647c2fff43b23a81f52d3a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sync/+/157680 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

gopherbot avatar Jan 13 '19 20:01 gopherbot

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla[bot] avatar Feb 24 '21 20:02 google-cla[bot]

This PR (HEAD: d5e48378160bf49067979cb4d8f840f39df6f033) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sync/+/157680 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

gopherbot avatar Feb 24 '21 20:02 gopherbot

Pull Request Updated and Synced with upstream 👍🏻

Fixes

  • golang/go#29721

Changes

Upstream changes introduced a change to the API Behavior in case of an acquire() that is bigger than the size of the semaphore.

  • Before: It was added to the queue (even though it was doomed to never get acquired)
  • Now: It returns an error.

In the case of introducing Resize(), The previous API behavior to block instead of returning an error makes sense, because an acquire() is no longer 100% doomed to never get acquired. As in the case of a resize that increases the semaphore's size enough the acquire() call can get unblocked.

I reverted the API to this previous behavior to block and wait if the size of acquire() is > size of semaphore.

sherifabdlnaby avatar Feb 24 '21 20:02 sherifabdlnaby

This PR (HEAD: 7c9f24f79d4fc66180ba1fd165c299cba13f547f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sync/+/157680 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off) See the Wiki page for more info

gopherbot avatar Feb 24 '21 20:02 gopherbot

What's the status of getting this PR merged into master?

thespags avatar Feb 05 '22 00:02 thespags

The proposal (#29721) was declined.

ianlancetaylor avatar Feb 05 '22 01:02 ianlancetaylor