tokio icon indicating copy to clipboard operation
tokio copied to clipboard

Semaphore::acquire_many API with `usize` count

Open danburkert opened this issue 3 years ago • 7 comments

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 avatar Jan 29 '22 21:01 danburkert

I've added a PR with the proposed new methods here: https://github.com/tokio-rs/tokio/pull/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.

danburkert avatar Jan 29 '22 22:01 danburkert

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 avatar Jan 30 '22 09:01 Darksonn

@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 avatar Jan 30 '22 20:01 danburkert

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.

danburkert avatar Jan 30 '22 20:01 danburkert

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

neonphog avatar May 25 '22 16:05 neonphog

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.

aaronriekenberg avatar Dec 27 '22 14:12 aaronriekenberg