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

Support larger number of permits #2607

Merged
merged 1 commit into from Jul 21, 2020
Merged

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Jun 10, 2020

I'm planning to use permits to limit peak memory usage, and for that u16 is very limiting.

I don't know if a small type is critical for performance here, so just in case I've opted for u32 instead of usize.

Related to #2605 #2598

@taiki-e taiki-e added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Jun 10, 2020
@taiki-e taiki-e requested a review from hawkw June 10, 2020 23:36
@kornelski kornelski force-pushed the morepermits branch 2 times, most recently from 1073aa6 to 4861ae7 Compare June 10, 2020 23:57
@Darksonn Darksonn added the C-enhancement Category: A PR with an enhancement or bugfix. label Jun 11, 2020
@Darksonn
Copy link
Contributor

It looks fine to me, but I would like to hear from @hawkw.

@Matthias247
Copy link
Contributor

Due to pub(crate) const MAX_PERMITS: usize = std::usize::MAX >> 3; that might not work on 32bit platforms, where you MAX_PERMITS would then be lower than u32::MAX. The counter which accumulates the permits as well as some other metadata in a single usize value could then underflow or overflow.

Maybe it could work if the u32 value is additionally checked against the maximum allowed value in some additional places - but I haven't checked that so far.

@kornelski
Copy link
Contributor Author

I've added an assertion and rebased.

This isn't yet exposed in this PR (#2598), so there are no more checks to add yet.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I believe the limit of a u16 was inherited from the previous Semaphore implementation (semaphore_ll) and shouldn't be necessary with the new implementation --- unless @carllerche, who wrote the old semaphore, can think of a reason it matters?

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-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants