Allow to configure RwLock max reads #3644
Conversation
zaharidichev
changed the title
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
| #[cfg(not(loom))] | ||
| const MAX_READS: usize = 32; | ||
| const MAX_READS: u32 = std::u32::MAX >> 3; |
There was a problem hiding this comment.
The other PR set this to batch_semaphore::MAX_PERMITS. How much larger/smaller is this?
There was a problem hiding this comment.
Well.. it is A LOT smaller than batch_semaphore::MAX_PERMITS. However, the idea is that we cannot acquire anything more than u32 from the semaphore so I think this is the right number. The shit mimics the one of batch_semaphore::MAX_PERMITS because on 32bit the last three bits are reserved for state. Does that sound correct?
There was a problem hiding this comment.
I mean, the number here is half a billion, so it's probably large enough. However, we were having some trouble in the other PR after increasing it. It would be nice to run this on a 32-bit machine to see if that also happens with this one.
There was a problem hiding this comment.
I think the problems with the other PR were due to this test timing out. Will try and run it on 32bit to verify though.
There was a problem hiding this comment.
@Darksonn I did run it on 32bit and it all seems fine. I think at the end the problem was at a faulty assert in the semaphore code.
zaharidichev
force-pushed
the
zd/rwlock-max-reads
branch
from
a7c94aa
to
a202d2c
Compare
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
tokio/src/sync/rwlock.rs
Outdated
| { | ||
| assert!( | ||
| max_reads <= MAX_READS, | ||
| "a RwLock may not be created with more than MAX_READS ({})", |
There was a problem hiding this comment.
| "a RwLock may not be created with more than MAX_READS ({})", | |
| "a RwLock may not be created with more than {} readers", |
| /// Creates a new instance of an `RwLock<T>` which is unlocked | ||
| /// and allows a maximum of `max_reads` concurrent readers. | ||
| /// | ||
| /// # Examples | ||
| /// |
There was a problem hiding this comment.
This should include a section on panics.
There was a problem hiding this comment.
Is it worth making the MAX_READS const public just so we can link to it, or we can just mention the exact number in the docs ?
There was a problem hiding this comment.
We don't make any of the other constants public. You can mention it as u32::MAX >> 3 or 2^29 - 1 as you prefer.
| @@ -271,6 +271,7 @@ impl Semaphore { | |||
| Self::MAX_PERMITS | |||
| ); | |||
| let prev = self.permits.fetch_add(rem << Self::PERMIT_SHIFT, Release); | |||
| let prev = prev >> Self::PERMIT_SHIFT; | |||
There was a problem hiding this comment.
This fix should probably be mentioned in the 1.5 changelog. I discovered this bug the hard way 😄
cc @Darksonn
There was a problem hiding this comment.
Thanks. Can you open a PR to modify the changelog?
Motivation
Currently
RwLockMAX_READSis capped to 32 which is too lowfor some usecases (see #3633).
Solution
This PR adds a constructor that allows for specifying the max reads.
It also bumps the default number of max reads from 32 to
u32::MAX >> 3Fix: #3633