Consider moving from parking_lot back to std locks #4610
Labels
C-Feature
A new feature, making something new possible
C-Performance
A change motivated by improving speed, memory usage or compile times
S-Blocked
This cannot move forward until something else changes
Comments
bjorn3
added
C-Feature
james7132
added
the
S-Blocked
|
I think this can be unblocked with the 1.62.0 release |
|
A reason why we might want to stick with parking_lot is its deadlock detection. Another option might be using the debug variants of tracing-mutex, which creates a DAG and ensures that there aren't any lock cycles (and the debug variants only do that for debug builds, but otherwise use the underlying std/parking_lot/etc primitives). |
github-merge-queue bot
pushed a commit
that referenced
this issue
Oct 2, 2023
# Objective - Fixes #4610 ## Solution - Replaced all instances of `parking_lot` locks with equivalents from `std::sync`. Acquiring locks within `std::sync` can fail, so `.expect("Lock Poisoned")` statements were added where required. ## Comments In [this comment](#4610 (comment)), the lack of deadlock detection was mentioned as a potential reason to not make this change. From what I can gather, Bevy doesn't appear to be using this functionality within the engine. Unless it was expected that a Bevy consumer was expected to enable and use this functionality, it appears to be a feature lost without consequence. Unfortunately, `cpal` and `wgpu` both still rely on `parking_lot`, leaving it in the dependency graph even after this change. From my basic experimentation, this change doesn't appear to have any performance impacts, positive or negative. I tested this using `bevymark` with 50,000 entities and observed 20ms of frame-time before and after the change. More extensive testing with larger/real projects should probably be done.
ameknite
pushed a commit
to ameknite/bevy
that referenced
this issue
Oct 3, 2023
# Objective - Fixes bevyengine#4610 ## Solution - Replaced all instances of `parking_lot` locks with equivalents from `std::sync`. Acquiring locks within `std::sync` can fail, so `.expect("Lock Poisoned")` statements were added where required. ## Comments In [this comment](bevyengine#4610 (comment)), the lack of deadlock detection was mentioned as a potential reason to not make this change. From what I can gather, Bevy doesn't appear to be using this functionality within the engine. Unless it was expected that a Bevy consumer was expected to enable and use this functionality, it appears to be a feature lost without consequence. Unfortunately, `cpal` and `wgpu` both still rely on `parking_lot`, leaving it in the dependency graph even after this change. From my basic experimentation, this change doesn't appear to have any performance impacts, positive or negative. I tested this using `bevymark` with 50,000 entities and observed 20ms of frame-time before and after the change. More extensive testing with larger/real projects should probably be done.
ameknite
pushed a commit
to ameknite/bevy
that referenced
this issue
Oct 3, 2023
# Objective - Fixes bevyengine#4610 ## Solution - Replaced all instances of `parking_lot` locks with equivalents from `std::sync`. Acquiring locks within `std::sync` can fail, so `.expect("Lock Poisoned")` statements were added where required. ## Comments In [this comment](bevyengine#4610 (comment)), the lack of deadlock detection was mentioned as a potential reason to not make this change. From what I can gather, Bevy doesn't appear to be using this functionality within the engine. Unless it was expected that a Bevy consumer was expected to enable and use this functionality, it appears to be a feature lost without consequence. Unfortunately, `cpal` and `wgpu` both still rely on `parking_lot`, leaving it in the dependency graph even after this change. From my basic experimentation, this change doesn't appear to have any performance impacts, positive or negative. I tested this using `bevymark` with 50,000 entities and observed 20ms of frame-time before and after the change. More extensive testing with larger/real projects should probably be done.
ameknite
pushed a commit
to ameknite/bevy
that referenced
this issue
Oct 3, 2023
# Objective - Fixes bevyengine#4610 ## Solution - Replaced all instances of `parking_lot` locks with equivalents from `std::sync`. Acquiring locks within `std::sync` can fail, so `.expect("Lock Poisoned")` statements were added where required. ## Comments In [this comment](bevyengine#4610 (comment)), the lack of deadlock detection was mentioned as a potential reason to not make this change. From what I can gather, Bevy doesn't appear to be using this functionality within the engine. Unless it was expected that a Bevy consumer was expected to enable and use this functionality, it appears to be a feature lost without consequence. Unfortunately, `cpal` and `wgpu` both still rely on `parking_lot`, leaving it in the dependency graph even after this change. From my basic experimentation, this change doesn't appear to have any performance impacts, positive or negative. I tested this using `bevymark` with 50,000 entities and observed 20ms of frame-time before and after the change. More extensive testing with larger/real projects should probably be done.
regnarock
pushed a commit
to regnarock/bevy
that referenced
this issue
Oct 13, 2023
# Objective - Fixes bevyengine#4610 ## Solution - Replaced all instances of `parking_lot` locks with equivalents from `std::sync`. Acquiring locks within `std::sync` can fail, so `.expect("Lock Poisoned")` statements were added where required. ## Comments In [this comment](bevyengine#4610 (comment)), the lack of deadlock detection was mentioned as a potential reason to not make this change. From what I can gather, Bevy doesn't appear to be using this functionality within the engine. Unless it was expected that a Bevy consumer was expected to enable and use this functionality, it appears to be a feature lost without consequence. Unfortunately, `cpal` and `wgpu` both still rely on `parking_lot`, leaving it in the dependency graph even after this change. From my basic experimentation, this change doesn't appear to have any performance impacts, positive or negative. I tested this using `bevymark` with 50,000 entities and observed 20ms of frame-time before and after the change. More extensive testing with larger/real projects should probably be done.
rdrpenguin04
pushed a commit
to rdrpenguin04/bevy
that referenced
this issue
Jan 9, 2024
# Objective - Fixes bevyengine#4610 ## Solution - Replaced all instances of `parking_lot` locks with equivalents from `std::sync`. Acquiring locks within `std::sync` can fail, so `.expect("Lock Poisoned")` statements were added where required. ## Comments In [this comment](bevyengine#4610 (comment)), the lack of deadlock detection was mentioned as a potential reason to not make this change. From what I can gather, Bevy doesn't appear to be using this functionality within the engine. Unless it was expected that a Bevy consumer was expected to enable and use this functionality, it appears to be a feature lost without consequence. Unfortunately, `cpal` and `wgpu` both still rely on `parking_lot`, leaving it in the dependency graph even after this change. From my basic experimentation, this change doesn't appear to have any performance impacts, positive or negative. I tested this using `bevymark` with 50,000 entities and observed 20ms of frame-time before and after the change. More extensive testing with larger/real projects should probably be done.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What problem does this solve or what need does it fill?
In the past the locks in libstd were wrappers around the OS native locks like provided by pthreads. Depending on the use case the locks of parking_lot are faster. For this reason we currently use parking_lot instead. As part of the work tracked in rust-lang/rust#93740 libstd is currently replacing OS native locks with custom implementations. The linux implementation is slightly slower than parking_lot when there is no contention, but it is faster than both parking_lot and the pthreads implementation of glibc and musl under light to extreme contention on at least two benchmarks (Amanieu/parking_lot#338 (comment) and rust-lang/rust#95035 (comment)).
What solution would you like?
We should benchmark the new locking implementation of libstd for the purposes of bevy and switch to them if they don't regress performance. Even if they don't improve
What alternative(s) have you considered?
Wait until rust-lang/rust#93740 is fully done.
The text was updated successfully, but these errors were encountered: