Improve performance and space of async_std::sync::Mutex #370
Conversation
nbdd0121
force-pushed
the
mutex
branch
4 times, most recently
from
50084a1 to
078294b
Compare
|
Some numbers Before this PR: After this PR: The PR also makes sure |
|
@stjepang This PR is ready. I scrapped the plan to squeeze everything into 1 usize (this PR makes it 2 usize), because squeezing further would hurt code clarity and the code would not be reusable for RwLock. |
Signed-off-by: Gary Guo <[email protected]>
All `Mutex`es now internally use `RawMutex` (which is similar to a `Mutex<()>`, only providing locking semantics but not data), therefore instantiating `Mutex`es on different types do not duplicate code. This patch does not otherwise change the algorithm used by `Mutex`. Signed-off-by: Gary Guo <[email protected]>
#[inline] are added to common and trivial functions, and slow paths are separated out from inlined hotpath. Signed-off-by: Gary Guo <[email protected]>
Signed-off-by: Gary Guo <[email protected]>
WakerListLock is an optimised version of Spinlock<WakerList> which is more efficient in performance and space. Signed-off-by: Gary Guo <[email protected]>
Signed-off-by: Gary Guo <[email protected]>
Signed-off-by: Gary Guo <[email protected]>
Moving the try_lock code to before touching the waker list is sound, because the waker list can only ever be accessed with `blocked` hold, so as long as we retry lock it while having `blocked` locked, we are okay. This code also set both LOCK and BLOCKED in the same atomic op. This has some performance improvements by touching the atomic variable 1 less time when inserting the entry. Signed-off-by: Gary Guo <[email protected]>
We originally check try_lock before test if opt_key is None. This commit changes its order. Doing so removes the need to deregister_waker when opt_key is None, therefore makes the hot path (un-contended case) faster. Signed-off-by: Gary Guo <[email protected]>
This makes RawLockFuture::poll itself small enough which is suitable for inlining. Signed-off-by: Gary Guo <[email protected]>
ghost
mentioned this pull request
| @@ -219,8 +304,9 @@ impl<T> Mutex<T> { | |||
| /// # | |||
| /// # }) | |||
| /// ``` | |||
| #[inline] | |||
There was a problem hiding this comment.
In general, forced inlining of trivial operations is considered an antipattern. Do we run into issues that these methods are not inlined?
There was a problem hiding this comment.
#[inline] is not forcing inline (different from #[inline(always)]; it is just hinting the compiler that this should probably be inlined.
Usually it's a good idea to have the #[inline] notation if the method is going to be inlined across the crate boundary. In the case Mutex is polymorphic over T, so we may get away with it, but a monomorphic function will never be inlined across crate boundary because the compiler cannot see the underlying implementation.
There was a problem hiding this comment.
Correct, I misspoke there a little.
That's correct as a hint, this is only in the absence of lto, though. Considering that libstd uses the same annotations, we should follow that practice.
|
@nbdd0121 we now have a Mutex implementation based on a WakerList, is this still needed? |
|
@dignifiedquire The currently implementation is still using Slab. It'll still be better to use a linked-list based implementation for predictable performance upper bound and avoid memory leak. However I'm quite busy recently. Maybe I'll take a look again and rebase next month. |
|
This would be no longer necessary after #822 lands, closing. |
Related #362