Closed AlexTMjugador closed 1 year ago
This is a good idea. I keep defaulting to parking lot because of the historical performance benefits and avoiding the unwraps :grimacing:
I would not mind submitting a PR with this change if it is deemed appropriate for the project heart
That would be great :heart_decoration:
It is conventional wisdom among the Rust community that
parking_lot
used to provide an implementation ofRwLock
that was better thanstd::sync::RwLock
:std
'sRwLock
always held a boxed a pthread lock, whileparking_lot
didn't.parking_lot
locks take 1 byte of storage space, instead of 1 word or more instd
's case.parking_lot
may explicitly take advantage of hardware lock elision instructions.parking_lot
locks do not poison on panic, while offering more features (mapped lock guards, upgradeable locks,serde
compatibility...) and predictable cross-platform behavior (due toparking_lot
implementing thread queuing data types in userspace).However, with the release of Rust 1.62, the
std::sync::RwLock
implementation was overhauled. The changes are described in detail at https://github.com/rust-lang/rust/issues/93740, its release announcement on the Rust blog, and the resources those pages link to.As it was discussed in the Rust language forum, I believe that the choice between
parking_lot
andstd::sync::RwLock
is less clear-cut now than it was before:std
's lock does no longer box any pthread lock on any Rust Tier 1 platform, barring macOS.parking_lot
is provided as a part of the Transactional Synchronization Extensions, which have been disabled in every Intel CPU that implements them via microcode updates to mitigate security issues. In addition, Intel considers such extensions deprecated, and stopped implementing them in newer processors. AMD never supported such extensions, AFAIK.std
's usage of OS-specific lock syscalls may improve thread scheduling decisions and, as a consequence, performance.parking_lot
claims to offer faster locks thanstd
in its README, but that claim hasn't been updated since 2016, so it can't take into account the improvements made onstd
for Rust 1.62.There are more arguments for and against both
std
's andparking_lot
locks, but I think the points above suffice to grasp that adding the ability to togglequick-cache
's dependency onparking_lot
may be a good idea. Moreover, it is a stated goal ofquick-cache
to have a "small dependency tree", so making dependencies optional increases the appeal of this crate with respect to that goal.As an experiment, I did a quick patch to https://github.com/arthurprs/quick-cache/commit/5dc07e188751ef9f04242ec2d28f398417a1c553 to swap
parking_lot
'sRwLock
withstd::sync::RwLock
and rancargo bench
before and after the change. Criterion did not find any statistically significant performance difference on a Linux system running on an Intel Core i7-12700 CPU.I would not mind submitting a PR with this change if it is deemed appropriate for the project :heart: