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

Consider making dependency on parking_lot optional #5

Closed
AlexTMjugador opened this issue Mar 19, 2023 · 1 comment · Fixed by #6
Closed

Consider making dependency on parking_lot optional #5

AlexTMjugador opened this issue Mar 19, 2023 · 1 comment · Fixed by #6

Comments

@AlexTMjugador
Copy link
Contributor

AlexTMjugador commented Mar 19, 2023

It is conventional wisdom among the Rust community that parking_lot used to provide an implementation of RwLock that was better than std::sync::RwLock:

  • std's RwLock always held a boxed a pthread lock, while parking_lot didn't.
  • parking_lot locks take 1 byte of storage space, instead of 1 word or more in std'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 to parking_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 rust-lang/rust#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 and std::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.
  • For applications that are not constrained by memory bandwidth or consumption, the execution speed difference between manipulating 1 byte vs. 1 word is likely to be minimal, due to how variables are aligned in memory and the fact that most processors in use today have word sizes greater than 1 byte.
  • In x86-64 CPUs, the hardware lock elision instruction used by 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.
  • Lock poisoning may be useful to handle shared data being left in an inconsistent state after a thread panics. In addition, 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 than std in its README, but that claim hasn't been updated since 2016, so it can't take into account the improvements made on std for Rust 1.62.

There are more arguments for and against both std's and parking_lot locks, but I think the points above suffice to grasp that adding the ability to toggle quick-cache's dependency on parking_lot may be a good idea. Moreover, it is a stated goal of quick-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 5dc07e1 to swap parking_lot's RwLock with std::sync::RwLock and ran cargo 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 ❤️

@arthurprs
Copy link
Owner

This is a good idea. I keep defaulting to parking lot because of the historical performance benefits and avoiding the unwraps 😬

I would not mind submitting a PR with this change if it is deemed appropriate for the project heart

That would be great 💟

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 a pull request may close this issue.

2 participants