-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Semaphore::acquire_many API with usize
count
#4446
Comments
I've added a PR with the proposed new methods here: #4447. I went with the names |
It's unfortunate that there's a mismatch in the types, but I'm not convinced its worth fixing it. Note that on 32-bit platforms, the maximum number of permits isn't 2^32, but rather 2^29. Similarly, on 64-bit, the maximum is 2^61. |
@Darksonn with the API as it is, it's not possible to use WRT the cost of fixing it, the semaphore internals are already in terms of |
It's situationally possible to extend the usefulness of the current |
👍 - am using a semaphore mapping to byte count, would be nice to not have the upper u32::MAX >> 3 ceiling. |
In this application: https://github.com/aaronriekenberg/rust-parallel/tree/0.1.20 I use a Semaphore to limit the number of parallel commands run. At the end I call Using This requires conversion from If |
+1 for this feature, We're also trying to use semaphores to limit disk space usage in a multi-threaded context. Is support for semaphores usize permit counts going to be added? |
Is your feature request related to a problem? Please describe.
The following
Semaphore
APIs use au32
count of permits:Semaphore::acquire_many
Semaphore::try_acquire_many
Semaphore::acquire_many_owned
Semaphore::try_acquire_many_owned
Whereas the following Semaphore APIs use a
usize
count of permits:Semaphore::new
Semaphore::const_new
Semaphore::available_permits
Semaphore::add_permits
This incongruity in the API is awkward, but it also prohibits a key usecase for the
Semaphore
type: protecting access to memory, e.g. memory leases. In fact the PR which added theu32
APIs mentions this usecase, but presumably the author didn't need to support leases of more than 4GiB (#2607).Describe the solution you'd like
[Owned]SemaphorePermit
to hold ausize
permit count internally instead ofu32
. I haven't confirmed, but I believe the size_ofSemaphorePermit
won't change due to padding, so hopefully this won't have any performance impact.[try_]acquire_many[_owned]
, and which take ausize
permit count. To kick off the painting, I propose potential names below.Semaphore::[try_]acquire_many[_owned]
.Describe alternatives you've considered
For applications which need to acquire more than 2^32 permits there really isn't a way to use the current semaphore API - it's not correct to acquire permits in a loop due to risk of deadlock.
Additional context
Potential names for the new methods:
[try_]acquire_many_permits[_owned]
[try_]acquire_permits[_owned]
[try_]acquire_n[_owned]
The text was updated successfully, but these errors were encountered: