- Notifications
You must be signed in to change notification settings - Fork341
Improve performance and space of async_std::sync::Mutex#370
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
50084a1 to078294bCompareSome 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 <gary@garyguo.net>
All `Mutex`es now internally use `RawMutex` (which is similar to a`Mutex<()>`, only providing locking semantics but not data), thereforeinstantiating `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 <gary@garyguo.net>
#[inline] are added to common and trivial functions, and slow paths areseparated out from inlined hotpath.Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
WakerListLock is an optimised version of Spinlock<WakerList> which ismore efficient in performance and space.Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Signed-off-by: Gary Guo <gary@garyguo.net>
Moving the try_lock code to before touching the waker list issound, because the waker list can only ever be accessed with `blocked`hold, so as long as we retry lock it while having `blocked` locked, weare okay.This code also set both LOCK and BLOCKED in the same atomic op. Thishas some performance improvements by touching the atomic variable 1less time when inserting the entry.Signed-off-by: Gary Guo <gary@garyguo.net>
We originally check try_lock before test if opt_key is None. This commitchanges its order. Doing so removes the need to deregister_waker whenopt_key is None, therefore makes the hot path (un-contended case) faster.Signed-off-by: Gary Guo <gary@garyguo.net>
This makes RawLockFuture::poll itself small enough which is suitablefor inlining.Signed-off-by: Gary Guo <gary@garyguo.net>
| @@ -219,8 +304,9 @@ impl<T> Mutex<T> { | |||
| /// # | |||
| /// # }) | |||
| /// ``` | |||
| #[inline] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
#[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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