Skip to content

Switch to using parking_lot::Mutex instead of std::sync::Mutex#1943

Merged
youknowone merged 1 commit intomasterfrom
coolreader18/switch-parking_lot-mutex
Jul 6, 2020
Merged

Switch to using parking_lot::Mutex instead of std::sync::Mutex#1943
youknowone merged 1 commit intomasterfrom
coolreader18/switch-parking_lot-mutex

Conversation

@coolreader18
Copy link
Copy Markdown
Member

All of the benefits of parking_lot::Mutex over the std one are listed in its
readme, but the ones that apply
to us are copied here:

  • Mutex and Once only require 1 byte of storage space, while Condvar and RwLock only require 1 word of storage space. On the other hand the standard library primitives require a dynamically allocated Box to hold OS-specific synchronization primitives. The small size of Mutex in particular encourages the use of fine-grained locks to increase parallelism.
    • less allocation is always good for performance, especially when all PyObjects are allocated and most require Mutexes
  • Uncontended lock acquisition and release is done through fast inline paths which only require a single atomic operation.
    • good, since a lot of Python code will only be run single-threaded
  • RwLock takes advantage of hardware lock elision on processors that support it, which can lead to huge performance wins with many readers.
    • requires nightly, but we could either build release binaries on nightly or wait until this is possible on stable
  • RwLock uses a task-fair locking policy, which avoids reader and writer starvation, whereas the standard library version makes no guarantees.
    • not sure if this is super necessary but it would definitely benefit the GIL-less experiment we're doing here

@palaviv
Copy link
Copy Markdown
Contributor

palaviv commented May 28, 2020

I think we should do such a big change only after we are using a published stable release of parking_lot.

@palaviv palaviv requested a review from youknowone May 28, 2020 10:11
@coolreader18
Copy link
Copy Markdown
Member Author

That sounds good to me, I can put this on hold till then.

@coolreader18 coolreader18 force-pushed the coolreader18/switch-parking_lot-mutex branch from 5faa2ac to ebab829 Compare July 5, 2020 03:58
@coolreader18
Copy link
Copy Markdown
Member Author

This gave about an 11% speedup for me running the test suite (though it's like 8-9 minutes so I didn't run hyperfine or anything yet) and a 4%-8% speedup for the benchmarks in the benchmarks directory.

@coolreader18
Copy link
Copy Markdown
Member Author

coolreader18 commented Jul 5, 2020

Actually, I've been thinking about something for a while that this would be a good change to make with: we could make a wrapper type around Arc/Mutex/RwLock or Rc/RefCell/RefCell, to allow for if somebody wants the performance and doesn't want threading. Then we could do something like PyRc, PyMutex, and PyRwLock and have the inner types be just implementation details.

Edit: if I do do this, it will be in a separate PR, it'd be best to make a batch change like this one on its own, I think.

@coolreader18
Copy link
Copy Markdown
Member Author

@youknowone could you review this?

Copy link
Copy Markdown
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Oh, I am really sorry to miss it. I didn't recognize I got review request of this PR.
I think this change is not necessary for now but no reason not to go if once already done.
It seems parking_lot is de facto standard in rust community.

@youknowone youknowone merged commit d497994 into master Jul 6, 2020
@youknowone youknowone deleted the coolreader18/switch-parking_lot-mutex branch July 6, 2020 08:54
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.

3 participants