From 33e416953d3d1a3eecc39cf4742b538664c8ed30 Mon Sep 17 00:00:00 2001 From: Zachary Harrold Date: Mon, 2 Oct 2023 23:44:34 +1100 Subject: [PATCH] Replaced `parking_lot` with `std::sync` (#9545) # 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](https://github.com/bevyengine/bevy/issues/4610#issuecomment-1592407881), 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. --- crates/bevy_audio/Cargo.toml | 1 - crates/bevy_reflect/Cargo.toml | 1 - crates/bevy_reflect/src/type_registry.rs | 20 ++++++++++++---- crates/bevy_reflect/src/utility.rs | 6 ++--- crates/bevy_render/Cargo.toml | 1 - crates/bevy_render/src/render_phase/draw.rs | 14 +++++++---- .../src/render_resource/pipeline_cache.rs | 24 +++++++++++++++---- crates/bevy_render/src/view/window/mod.rs | 12 ++++++++-- .../bevy_render/src/view/window/screenshot.rs | 5 ++-- 9 files changed, 60 insertions(+), 24 deletions(-) diff --git a/crates/bevy_audio/Cargo.toml b/crates/bevy_audio/Cargo.toml index b905ebf88ac2c5..522c44de5f4f2d 100644 --- a/crates/bevy_audio/Cargo.toml +++ b/crates/bevy_audio/Cargo.toml @@ -21,7 +21,6 @@ bevy_utils = { path = "../bevy_utils", version = "0.12.0-dev" } # other rodio = { version = "0.17", default-features = false } -parking_lot = "0.12.1" [target.'cfg(target_os = "android")'.dependencies] oboe = { version = "0.5", optional = true } diff --git a/crates/bevy_reflect/Cargo.toml b/crates/bevy_reflect/Cargo.toml index 33b4ad6a1cf0ce..96884618458ee0 100644 --- a/crates/bevy_reflect/Cargo.toml +++ b/crates/bevy_reflect/Cargo.toml @@ -28,7 +28,6 @@ bevy_ptr = { path = "../bevy_ptr", version = "0.12.0-dev" } # other erased-serde = "0.3" downcast-rs = "1.2" -parking_lot = "0.12.1" thiserror = "1.0" once_cell = "1.11" serde = "1" diff --git a/crates/bevy_reflect/src/type_registry.rs b/crates/bevy_reflect/src/type_registry.rs index f178108eeb249c..4e7ff1d398f5cc 100644 --- a/crates/bevy_reflect/src/type_registry.rs +++ b/crates/bevy_reflect/src/type_registry.rs @@ -2,9 +2,12 @@ use crate::{serde::Serializable, Reflect, TypeInfo, Typed}; use bevy_ptr::{Ptr, PtrMut}; use bevy_utils::{HashMap, HashSet}; use downcast_rs::{impl_downcast, Downcast}; -use parking_lot::{RwLock, RwLockReadGuard, RwLockWriteGuard}; use serde::Deserialize; -use std::{any::TypeId, fmt::Debug, sync::Arc}; +use std::{ + any::TypeId, + fmt::Debug, + sync::{Arc, PoisonError, RwLock, RwLockReadGuard, RwLockWriteGuard}, +}; /// A registry of [reflected] types. /// @@ -35,7 +38,12 @@ pub struct TypeRegistryArc { impl Debug for TypeRegistryArc { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.internal.read().full_name_to_id.keys().fmt(f) + self.internal + .read() + .unwrap_or_else(PoisonError::into_inner) + .full_name_to_id + .keys() + .fmt(f) } } @@ -267,12 +275,14 @@ impl TypeRegistry { impl TypeRegistryArc { /// Takes a read lock on the underlying [`TypeRegistry`]. pub fn read(&self) -> RwLockReadGuard<'_, TypeRegistry> { - self.internal.read() + self.internal.read().unwrap_or_else(PoisonError::into_inner) } /// Takes a write lock on the underlying [`TypeRegistry`]. pub fn write(&self) -> RwLockWriteGuard<'_, TypeRegistry> { - self.internal.write() + self.internal + .write() + .unwrap_or_else(PoisonError::into_inner) } } diff --git a/crates/bevy_reflect/src/utility.rs b/crates/bevy_reflect/src/utility.rs index e1072841d5b683..d79410b1409466 100644 --- a/crates/bevy_reflect/src/utility.rs +++ b/crates/bevy_reflect/src/utility.rs @@ -3,10 +3,10 @@ use crate::TypeInfo; use bevy_utils::{FixedState, StableHashMap}; use once_cell::race::OnceBox; -use parking_lot::RwLock; use std::{ any::{Any, TypeId}, hash::BuildHasher, + sync::{PoisonError, RwLock}, }; /// A type that can be stored in a ([`Non`])[`GenericTypeCell`]. @@ -236,7 +236,7 @@ impl GenericTypeCell { // since `f` might want to call `get_or_insert` recursively // and we don't want a deadlock! { - let mapping = self.0.read(); + let mapping = self.0.read().unwrap_or_else(PoisonError::into_inner); if let Some(info) = mapping.get(&type_id) { return info; } @@ -244,7 +244,7 @@ impl GenericTypeCell { let value = f(); - let mut mapping = self.0.write(); + let mut mapping = self.0.write().unwrap_or_else(PoisonError::into_inner); mapping .entry(type_id) .insert({ diff --git a/crates/bevy_render/Cargo.toml b/crates/bevy_render/Cargo.toml index 32573be7b9a119..3764318ea62240 100644 --- a/crates/bevy_render/Cargo.toml +++ b/crates/bevy_render/Cargo.toml @@ -69,7 +69,6 @@ thread_local = "1.1" thiserror = "1.0" futures-lite = "1.4.0" hexasphere = "9.0" -parking_lot = "0.12.1" ddsfile = { version = "0.5.0", optional = true } ktx2 = { version = "0.3.0", optional = true } naga_oil = "0.8" diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index 405467ebeccc0b..1d439e501db448 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -7,8 +7,12 @@ use bevy_ecs::{ world::World, }; use bevy_utils::{all_tuples, HashMap}; -use parking_lot::{RwLock, RwLockReadGuard, RwLockWriteGuard}; -use std::{any::TypeId, fmt::Debug, hash::Hash}; +use std::{ + any::TypeId, + fmt::Debug, + hash::Hash, + sync::{PoisonError, RwLock, RwLockReadGuard, RwLockWriteGuard}, +}; /// A draw function used to draw [`PhaseItem`]s. /// @@ -116,12 +120,14 @@ impl Default for DrawFunctions

{ impl DrawFunctions

{ /// Accesses the draw functions in read mode. pub fn read(&self) -> RwLockReadGuard<'_, DrawFunctionsInternal

> { - self.internal.read() + self.internal.read().unwrap_or_else(PoisonError::into_inner) } /// Accesses the draw functions in write mode. pub fn write(&self) -> RwLockWriteGuard<'_, DrawFunctionsInternal

> { - self.internal.write() + self.internal + .write() + .unwrap_or_else(PoisonError::into_inner) } } diff --git a/crates/bevy_render/src/render_resource/pipeline_cache.rs b/crates/bevy_render/src/render_resource/pipeline_cache.rs index c9d643fb7ff6c6..3eb6f8ae43ef97 100644 --- a/crates/bevy_render/src/render_resource/pipeline_cache.rs +++ b/crates/bevy_render/src/render_resource/pipeline_cache.rs @@ -16,8 +16,13 @@ use bevy_utils::{ Entry, HashMap, HashSet, }; use naga::valid::Capabilities; -use parking_lot::Mutex; -use std::{borrow::Cow, hash::Hash, mem, ops::Deref}; +use std::{ + borrow::Cow, + hash::Hash, + mem, + ops::Deref, + sync::{Mutex, PoisonError}, +}; use thiserror::Error; #[cfg(feature = "shader_format_spirv")] use wgpu::util::make_spirv; @@ -587,7 +592,10 @@ impl PipelineCache { &self, descriptor: RenderPipelineDescriptor, ) -> CachedRenderPipelineId { - let mut new_pipelines = self.new_pipelines.lock(); + let mut new_pipelines = self + .new_pipelines + .lock() + .unwrap_or_else(PoisonError::into_inner); let id = CachedRenderPipelineId(self.pipelines.len() + new_pipelines.len()); new_pipelines.push(CachedPipeline { descriptor: PipelineDescriptor::RenderPipelineDescriptor(Box::new(descriptor)), @@ -613,7 +621,10 @@ impl PipelineCache { &self, descriptor: ComputePipelineDescriptor, ) -> CachedComputePipelineId { - let mut new_pipelines = self.new_pipelines.lock(); + let mut new_pipelines = self + .new_pipelines + .lock() + .unwrap_or_else(PoisonError::into_inner); let id = CachedComputePipelineId(self.pipelines.len() + new_pipelines.len()); new_pipelines.push(CachedPipeline { descriptor: PipelineDescriptor::ComputePipelineDescriptor(Box::new(descriptor)), @@ -773,7 +784,10 @@ impl PipelineCache { let mut pipelines = mem::take(&mut self.pipelines); { - let mut new_pipelines = self.new_pipelines.lock(); + let mut new_pipelines = self + .new_pipelines + .lock() + .unwrap_or_else(PoisonError::into_inner); for new_pipeline in new_pipelines.drain(..) { let id = pipelines.len(); pipelines.push(new_pipeline); diff --git a/crates/bevy_render/src/view/window/mod.rs b/crates/bevy_render/src/view/window/mod.rs index 88820f85660966..d0f38a3789f523 100644 --- a/crates/bevy_render/src/view/window/mod.rs +++ b/crates/bevy_render/src/view/window/mod.rs @@ -10,7 +10,10 @@ use bevy_utils::{default, tracing::debug, HashMap, HashSet}; use bevy_window::{ CompositeAlphaMode, PresentMode, PrimaryWindow, RawHandleWrapper, Window, WindowClosed, }; -use std::ops::{Deref, DerefMut}; +use std::{ + ops::{Deref, DerefMut}, + sync::PoisonError, +}; use wgpu::{BufferUsages, TextureFormat, TextureUsages, TextureViewDescriptor}; pub mod screenshot; @@ -168,7 +171,12 @@ fn extract_windows( // Even if a user had multiple copies of this system, since the system has a mutable resource access the two systems would never run // at the same time // 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? - for (window, screenshot_func) in screenshot_manager.callbacks.lock().drain() { + for (window, screenshot_func) in screenshot_manager + .callbacks + .lock() + .unwrap_or_else(PoisonError::into_inner) + .drain() + { if let Some(window) = extracted_windows.get_mut(&window) { window.screenshot_func = Some(screenshot_func); } diff --git a/crates/bevy_render/src/view/window/screenshot.rs b/crates/bevy_render/src/view/window/screenshot.rs index 6907cd690ca920..db3a034744be06 100644 --- a/crates/bevy_render/src/view/window/screenshot.rs +++ b/crates/bevy_render/src/view/window/screenshot.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, path::Path}; +use std::{borrow::Cow, path::Path, sync::PoisonError}; use bevy_app::Plugin; use bevy_asset::{load_internal_asset, Handle}; @@ -6,7 +6,7 @@ use bevy_ecs::prelude::*; use bevy_log::{error, info, info_span}; use bevy_tasks::AsyncComputeTaskPool; use bevy_utils::HashMap; -use parking_lot::Mutex; +use std::sync::Mutex; use thiserror::Error; use wgpu::{ CommandEncoder, Extent3d, ImageDataLayout, TextureFormat, COPY_BYTES_PER_ROW_ALIGNMENT, @@ -50,6 +50,7 @@ impl ScreenshotManager { ) -> Result<(), ScreenshotAlreadyRequestedError> { self.callbacks .get_mut() + .unwrap_or_else(PoisonError::into_inner) .try_insert(window, Box::new(callback)) .map(|_| ()) .map_err(|_| ScreenshotAlreadyRequestedError)