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

Semaphore::acquire_many API with usize count #4446

Open
danburkert opened this issue Jan 29, 2022 · 7 comments
Open

Semaphore::acquire_many API with usize count #4446

danburkert opened this issue Jan 29, 2022 · 7 comments
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-sync Module: tokio/sync

Comments

@danburkert
Copy link
Contributor

Is your feature request related to a problem? Please describe.

The following Semaphore APIs use a u32 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 the u32 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

  1. Change [Owned]SemaphorePermit to hold a usize permit count internally instead of u32. I haven't confirmed, but I believe the size_of SemaphorePermit won't change due to padding, so hopefully this won't have any performance impact.
  2. Add four new methods replacing each permutation of [try_]acquire_many[_owned], and which take a usize permit count. To kick off the painting, I propose potential names below.
  3. [optional] Deprecate 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]
@danburkert danburkert added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Jan 29, 2022
@danburkert
Copy link
Contributor Author

I've added a PR with the proposed new methods here: #4447. I went with the names [try_]acquire_permits[_owned], but definitely open to changing that. I did not add deprecation tags to the original methods, but can if that's the consensus.

@Darksonn Darksonn added the M-sync Module: tokio/sync label Jan 30, 2022
@Darksonn
Copy link
Contributor

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.

@danburkert
Copy link
Contributor Author

@Darksonn with the API as it is, it's not possible to use Semaphore to implement memory leases with support for leases > 4GiB. Memory leases of this style are a common technique data analysis systems, warehousing, query execution, etc. For these usecases a 4GiB limit is orders of magnitude too low. The 2^61 limit is not an issue since no practical system is approaching that much memory, and x86-64 is typically limited to 52 bytes of addressable space anyway.

WRT the cost of fixing it, the semaphore internals are already in terms of usize so it's not a major change.

@danburkert
Copy link
Contributor Author

It's situationally possible to extend the usefulness of the current Semaphore type by mapping a lease to a page of memory instead of a byte, yielding a leases of up to 2 ^ 44 = 16 TiB, which is fine for most systems now, but perhaps only for a few more years.

@neonphog
Copy link

👍 - am using a semaphore mapping to byte count, would be nice to not have the upper u32::MAX >> 3 ceiling.

@aaronriekenberg
Copy link

aaronriekenberg commented Dec 27, 2022

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 acquire_many to wait for all command tasks to complete: https://github.com/aaronriekenberg/rust-parallel/blob/0.1.20/src/command.rs#L212

Using u32 for --jobs/-j command line argument to match type of acquire_many: https://github.com/aaronriekenberg/rust-parallel/blob/0.1.20/src/command_line_args.rs#L15-L17

This requires conversion from num_cpus::get() which returns usize. Also requires conversion when creating the Semaphore: https://github.com/aaronriekenberg/rust-parallel/blob/0.1.20/src/command.rs#L79-L82

If acquire_many took a usize parameter I could use usize for --jobs/-j parameter. Would eliminate conversions and be cleaner for this use case.

@joe-amzn
Copy link

joe-amzn commented May 10, 2024

+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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-sync Module: tokio/sync
Projects
None yet
Development

No branches or pull requests

5 participants