Skip to content

Commit 08e069d

Browse files
bushrat011899Ray Redondo
authored and
Ray Redondo
committed
Replaced parking_lot with std::sync (bevyengine#9545)
# 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.
1 parent 662166b commit 08e069d

File tree

9 files changed

+60
-24
lines changed

9 files changed

+60
-24
lines changed

crates/bevy_audio/Cargo.toml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ bevy_utils = { path = "../bevy_utils", version = "0.12.0-dev" }
2121

2222
# other
2323
rodio = { version = "0.17", default-features = false }
24-
parking_lot = "0.12.1"
2524

2625
[target.'cfg(target_os = "android")'.dependencies]
2726
oboe = { version = "0.5", optional = true }

crates/bevy_reflect/Cargo.toml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ bevy_ptr = { path = "../bevy_ptr", version = "0.12.0-dev" }
2828
# other
2929
erased-serde = "0.3"
3030
downcast-rs = "1.2"
31-
parking_lot = "0.12.1"
3231
thiserror = "1.0"
3332
once_cell = "1.11"
3433
serde = "1"

crates/bevy_reflect/src/type_registry.rs

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,12 @@ use crate::{serde::Serializable, Reflect, TypeInfo, Typed};
22
use bevy_ptr::{Ptr, PtrMut};
33
use bevy_utils::{HashMap, HashSet};
44
use downcast_rs::{impl_downcast, Downcast};
5-
use parking_lot::{RwLock, RwLockReadGuard, RwLockWriteGuard};
65
use serde::Deserialize;
7-
use std::{any::TypeId, fmt::Debug, sync::Arc};
6+
use std::{
7+
any::TypeId,
8+
fmt::Debug,
9+
sync::{Arc, PoisonError, RwLock, RwLockReadGuard, RwLockWriteGuard},
10+
};
811

912
/// A registry of [reflected] types.
1013
///
@@ -35,7 +38,12 @@ pub struct TypeRegistryArc {
3538

3639
impl Debug for TypeRegistryArc {
3740
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
38-
self.internal.read().full_name_to_id.keys().fmt(f)
41+
self.internal
42+
.read()
43+
.unwrap_or_else(PoisonError::into_inner)
44+
.full_name_to_id
45+
.keys()
46+
.fmt(f)
3947
}
4048
}
4149

@@ -267,12 +275,14 @@ impl TypeRegistry {
267275
impl TypeRegistryArc {
268276
/// Takes a read lock on the underlying [`TypeRegistry`].
269277
pub fn read(&self) -> RwLockReadGuard<'_, TypeRegistry> {
270-
self.internal.read()
278+
self.internal.read().unwrap_or_else(PoisonError::into_inner)
271279
}
272280

273281
/// Takes a write lock on the underlying [`TypeRegistry`].
274282
pub fn write(&self) -> RwLockWriteGuard<'_, TypeRegistry> {
275-
self.internal.write()
283+
self.internal
284+
.write()
285+
.unwrap_or_else(PoisonError::into_inner)
276286
}
277287
}
278288

crates/bevy_reflect/src/utility.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@
33
use crate::TypeInfo;
44
use bevy_utils::{FixedState, StableHashMap};
55
use once_cell::race::OnceBox;
6-
use parking_lot::RwLock;
76
use std::{
87
any::{Any, TypeId},
98
hash::BuildHasher,
9+
sync::{PoisonError, RwLock},
1010
};
1111

1212
/// A type that can be stored in a ([`Non`])[`GenericTypeCell`].
@@ -236,15 +236,15 @@ impl<T: TypedProperty> GenericTypeCell<T> {
236236
// since `f` might want to call `get_or_insert` recursively
237237
// and we don't want a deadlock!
238238
{
239-
let mapping = self.0.read();
239+
let mapping = self.0.read().unwrap_or_else(PoisonError::into_inner);
240240
if let Some(info) = mapping.get(&type_id) {
241241
return info;
242242
}
243243
}
244244

245245
let value = f();
246246

247-
let mut mapping = self.0.write();
247+
let mut mapping = self.0.write().unwrap_or_else(PoisonError::into_inner);
248248
mapping
249249
.entry(type_id)
250250
.insert({

crates/bevy_render/Cargo.toml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@ thread_local = "1.1"
6969
thiserror = "1.0"
7070
futures-lite = "1.4.0"
7171
hexasphere = "9.0"
72-
parking_lot = "0.12.1"
7372
ddsfile = { version = "0.5.0", optional = true }
7473
ktx2 = { version = "0.3.0", optional = true }
7574
naga_oil = "0.8"

crates/bevy_render/src/render_phase/draw.rs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,12 @@ use bevy_ecs::{
77
world::World,
88
};
99
use bevy_utils::{all_tuples, HashMap};
10-
use parking_lot::{RwLock, RwLockReadGuard, RwLockWriteGuard};
11-
use std::{any::TypeId, fmt::Debug, hash::Hash};
10+
use std::{
11+
any::TypeId,
12+
fmt::Debug,
13+
hash::Hash,
14+
sync::{PoisonError, RwLock, RwLockReadGuard, RwLockWriteGuard},
15+
};
1216

1317
/// A draw function used to draw [`PhaseItem`]s.
1418
///
@@ -116,12 +120,14 @@ impl<P: PhaseItem> Default for DrawFunctions<P> {
116120
impl<P: PhaseItem> DrawFunctions<P> {
117121
/// Accesses the draw functions in read mode.
118122
pub fn read(&self) -> RwLockReadGuard<'_, DrawFunctionsInternal<P>> {
119-
self.internal.read()
123+
self.internal.read().unwrap_or_else(PoisonError::into_inner)
120124
}
121125

122126
/// Accesses the draw functions in write mode.
123127
pub fn write(&self) -> RwLockWriteGuard<'_, DrawFunctionsInternal<P>> {
124-
self.internal.write()
128+
self.internal
129+
.write()
130+
.unwrap_or_else(PoisonError::into_inner)
125131
}
126132
}
127133

crates/bevy_render/src/render_resource/pipeline_cache.rs

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,13 @@ use bevy_utils::{
1616
Entry, HashMap, HashSet,
1717
};
1818
use naga::valid::Capabilities;
19-
use parking_lot::Mutex;
20-
use std::{borrow::Cow, hash::Hash, mem, ops::Deref};
19+
use std::{
20+
borrow::Cow,
21+
hash::Hash,
22+
mem,
23+
ops::Deref,
24+
sync::{Mutex, PoisonError},
25+
};
2126
use thiserror::Error;
2227
#[cfg(feature = "shader_format_spirv")]
2328
use wgpu::util::make_spirv;
@@ -587,7 +592,10 @@ impl PipelineCache {
587592
&self,
588593
descriptor: RenderPipelineDescriptor,
589594
) -> CachedRenderPipelineId {
590-
let mut new_pipelines = self.new_pipelines.lock();
595+
let mut new_pipelines = self
596+
.new_pipelines
597+
.lock()
598+
.unwrap_or_else(PoisonError::into_inner);
591599
let id = CachedRenderPipelineId(self.pipelines.len() + new_pipelines.len());
592600
new_pipelines.push(CachedPipeline {
593601
descriptor: PipelineDescriptor::RenderPipelineDescriptor(Box::new(descriptor)),
@@ -613,7 +621,10 @@ impl PipelineCache {
613621
&self,
614622
descriptor: ComputePipelineDescriptor,
615623
) -> CachedComputePipelineId {
616-
let mut new_pipelines = self.new_pipelines.lock();
624+
let mut new_pipelines = self
625+
.new_pipelines
626+
.lock()
627+
.unwrap_or_else(PoisonError::into_inner);
617628
let id = CachedComputePipelineId(self.pipelines.len() + new_pipelines.len());
618629
new_pipelines.push(CachedPipeline {
619630
descriptor: PipelineDescriptor::ComputePipelineDescriptor(Box::new(descriptor)),
@@ -773,7 +784,10 @@ impl PipelineCache {
773784
let mut pipelines = mem::take(&mut self.pipelines);
774785

775786
{
776-
let mut new_pipelines = self.new_pipelines.lock();
787+
let mut new_pipelines = self
788+
.new_pipelines
789+
.lock()
790+
.unwrap_or_else(PoisonError::into_inner);
777791
for new_pipeline in new_pipelines.drain(..) {
778792
let id = pipelines.len();
779793
pipelines.push(new_pipeline);

crates/bevy_render/src/view/window/mod.rs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@ use bevy_utils::{default, tracing::debug, HashMap, HashSet};
1010
use bevy_window::{
1111
CompositeAlphaMode, PresentMode, PrimaryWindow, RawHandleWrapper, Window, WindowClosed,
1212
};
13-
use std::ops::{Deref, DerefMut};
13+
use std::{
14+
ops::{Deref, DerefMut},
15+
sync::PoisonError,
16+
};
1417
use wgpu::{BufferUsages, TextureFormat, TextureUsages, TextureViewDescriptor};
1518

1619
pub mod screenshot;
@@ -168,7 +171,12 @@ fn extract_windows(
168171
// Even if a user had multiple copies of this system, since the system has a mutable resource access the two systems would never run
169172
// at the same time
170173
// TODO: since this is guaranteed, should the lock be replaced with an UnsafeCell to remove the overhead, or is it minor enough to be ignored?
171-
for (window, screenshot_func) in screenshot_manager.callbacks.lock().drain() {
174+
for (window, screenshot_func) in screenshot_manager
175+
.callbacks
176+
.lock()
177+
.unwrap_or_else(PoisonError::into_inner)
178+
.drain()
179+
{
172180
if let Some(window) = extracted_windows.get_mut(&window) {
173181
window.screenshot_func = Some(screenshot_func);
174182
}

crates/bevy_render/src/view/window/screenshot.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
use std::{borrow::Cow, path::Path};
1+
use std::{borrow::Cow, path::Path, sync::PoisonError};
22

33
use bevy_app::Plugin;
44
use bevy_asset::{load_internal_asset, Handle};
55
use bevy_ecs::prelude::*;
66
use bevy_log::{error, info, info_span};
77
use bevy_tasks::AsyncComputeTaskPool;
88
use bevy_utils::HashMap;
9-
use parking_lot::Mutex;
9+
use std::sync::Mutex;
1010
use thiserror::Error;
1111
use wgpu::{
1212
CommandEncoder, Extent3d, ImageDataLayout, TextureFormat, COPY_BYTES_PER_ROW_ALIGNMENT,
@@ -50,6 +50,7 @@ impl ScreenshotManager {
5050
) -> Result<(), ScreenshotAlreadyRequestedError> {
5151
self.callbacks
5252
.get_mut()
53+
.unwrap_or_else(PoisonError::into_inner)
5354
.try_insert(window, Box::new(callback))
5455
.map(|_| ())
5556
.map_err(|_| ScreenshotAlreadyRequestedError)

0 commit comments

Comments
 (0)