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

feat: use parking_lot::Mutex #95

Merged
merged 2 commits into from Nov 6, 2020

Conversation

jhgg
Copy link
Collaborator

@jhgg jhgg commented Oct 30, 2020

parking_lot::Mutex is faster than std::sync::Mutex, so let's use it for the critical lock in CassFuture.

This section of parking_lot's docs describes the difference: https://docs.rs/parking_lot/0.11.0/parking_lot/type.Mutex.html#differences-from-the-standard-library-mutex

Signed-off-by: jake <jh@discordapp.com>
@kw217
Copy link
Collaborator

kw217 commented Nov 4, 2020

Is there a need for this? I just read faern/parking_lot#1 and rust-lang/rust#56410 and it seems like parking_lot is still a fair way off being considered for the standard library, due to various worries about correctness and developer documentation.

@jhgg
Copy link
Collaborator Author

jhgg commented Nov 5, 2020

The main motivation is that it's faster under the uncontended case (which is the majority here), and namely, our use of std::Mutex is incorrect anyways as we potentially can panic (although this is super unlikely) from a CFFI function which to my understanding is UB. (panic being unwrapping a poisoned mutex), https://doc.rust-lang.org/nomicon/ffi.html#ffi-and-panics

@jhgg
Copy link
Collaborator Author

jhgg commented Nov 5, 2020

anyways - we also run parking_lot in prod without any issue. so aside from a slight increase in compile time this should purely be "faster".

Copy link
Collaborator

@kw217 kw217 left a comment

Choose a reason for hiding this comment

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

OK. I see lots of other projects are using parking-lot, and your point about unwrap in CFFI is a very good one.

Code LGTM - thanks for driving this.

@kw217
Copy link
Collaborator

kw217 commented Nov 5, 2020

As with the other ones, this is now out-of-date so needs you to click "update". Best to just do one at a time. I'll try and check regularly during 0900-1700 UTC - is there any overlap with your hours?

@jhgg
Copy link
Collaborator Author

jhgg commented Nov 5, 2020

Updated.

@kw217 kw217 merged commit 491a586 into Metaswitch:master Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants