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.
This commit is contained in:
Zachary Harrold 2023-10-02 23:44:34 +11:00 committed by GitHub
parent 44c769f7b9
commit 450328d15e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 60 additions and 24 deletions

View File

@ -21,7 +21,6 @@ bevy_utils = { path = "../bevy_utils", version = "0.12.0-dev" }
# other # other
rodio = { version = "0.17", default-features = false } rodio = { version = "0.17", default-features = false }
parking_lot = "0.12.1"
[target.'cfg(target_os = "android")'.dependencies] [target.'cfg(target_os = "android")'.dependencies]
oboe = { version = "0.5", optional = true } oboe = { version = "0.5", optional = true }

View File

@ -28,7 +28,6 @@ bevy_ptr = { path = "../bevy_ptr", version = "0.12.0-dev" }
# other # other
erased-serde = "0.3" erased-serde = "0.3"
downcast-rs = "1.2" downcast-rs = "1.2"
parking_lot = "0.12.1"
thiserror = "1.0" thiserror = "1.0"
once_cell = "1.11" once_cell = "1.11"
serde = "1" serde = "1"

View File

@ -2,9 +2,12 @@ use crate::{serde::Serializable, Reflect, TypeInfo, Typed};
use bevy_ptr::{Ptr, PtrMut}; use bevy_ptr::{Ptr, PtrMut};
use bevy_utils::{HashMap, HashSet}; use bevy_utils::{HashMap, HashSet};
use downcast_rs::{impl_downcast, Downcast}; use downcast_rs::{impl_downcast, Downcast};
use parking_lot::{RwLock, RwLockReadGuard, RwLockWriteGuard};
use serde::Deserialize; 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. /// A registry of [reflected] types.
/// ///
@ -35,7 +38,12 @@ pub struct TypeRegistryArc {
impl Debug for TypeRegistryArc { impl Debug for TypeRegistryArc {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { 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 { impl TypeRegistryArc {
/// Takes a read lock on the underlying [`TypeRegistry`]. /// Takes a read lock on the underlying [`TypeRegistry`].
pub fn read(&self) -> RwLockReadGuard<'_, 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`]. /// Takes a write lock on the underlying [`TypeRegistry`].
pub fn write(&self) -> RwLockWriteGuard<'_, TypeRegistry> { pub fn write(&self) -> RwLockWriteGuard<'_, TypeRegistry> {
self.internal.write() self.internal
.write()
.unwrap_or_else(PoisonError::into_inner)
} }
} }

View File

@ -3,10 +3,10 @@
use crate::TypeInfo; use crate::TypeInfo;
use bevy_utils::{FixedState, StableHashMap}; use bevy_utils::{FixedState, StableHashMap};
use once_cell::race::OnceBox; use once_cell::race::OnceBox;
use parking_lot::RwLock;
use std::{ use std::{
any::{Any, TypeId}, any::{Any, TypeId},
hash::BuildHasher, hash::BuildHasher,
sync::{PoisonError, RwLock},
}; };
/// A type that can be stored in a ([`Non`])[`GenericTypeCell`]. /// A type that can be stored in a ([`Non`])[`GenericTypeCell`].
@ -236,7 +236,7 @@ impl<T: TypedProperty> GenericTypeCell<T> {
// since `f` might want to call `get_or_insert` recursively // since `f` might want to call `get_or_insert` recursively
// and we don't want a deadlock! // 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) { if let Some(info) = mapping.get(&type_id) {
return info; return info;
} }
@ -244,7 +244,7 @@ impl<T: TypedProperty> GenericTypeCell<T> {
let value = f(); let value = f();
let mut mapping = self.0.write(); let mut mapping = self.0.write().unwrap_or_else(PoisonError::into_inner);
mapping mapping
.entry(type_id) .entry(type_id)
.insert({ .insert({

View File

@ -69,7 +69,6 @@ thread_local = "1.1"
thiserror = "1.0" thiserror = "1.0"
futures-lite = "1.4.0" futures-lite = "1.4.0"
hexasphere = "9.0" hexasphere = "9.0"
parking_lot = "0.12.1"
ddsfile = { version = "0.5.0", optional = true } ddsfile = { version = "0.5.0", optional = true }
ktx2 = { version = "0.3.0", optional = true } ktx2 = { version = "0.3.0", optional = true }
naga_oil = "0.8" naga_oil = "0.8"

View File

@ -7,8 +7,12 @@ use bevy_ecs::{
world::World, world::World,
}; };
use bevy_utils::{all_tuples, HashMap}; use bevy_utils::{all_tuples, HashMap};
use parking_lot::{RwLock, RwLockReadGuard, RwLockWriteGuard}; use std::{
use std::{any::TypeId, fmt::Debug, hash::Hash}; any::TypeId,
fmt::Debug,
hash::Hash,
sync::{PoisonError, RwLock, RwLockReadGuard, RwLockWriteGuard},
};
/// A draw function used to draw [`PhaseItem`]s. /// A draw function used to draw [`PhaseItem`]s.
/// ///
@ -116,12 +120,14 @@ impl<P: PhaseItem> Default for DrawFunctions<P> {
impl<P: PhaseItem> DrawFunctions<P> { impl<P: PhaseItem> DrawFunctions<P> {
/// Accesses the draw functions in read mode. /// Accesses the draw functions in read mode.
pub fn read(&self) -> RwLockReadGuard<'_, DrawFunctionsInternal<P>> { pub fn read(&self) -> RwLockReadGuard<'_, DrawFunctionsInternal<P>> {
self.internal.read() self.internal.read().unwrap_or_else(PoisonError::into_inner)
} }
/// Accesses the draw functions in write mode. /// Accesses the draw functions in write mode.
pub fn write(&self) -> RwLockWriteGuard<'_, DrawFunctionsInternal<P>> { pub fn write(&self) -> RwLockWriteGuard<'_, DrawFunctionsInternal<P>> {
self.internal.write() self.internal
.write()
.unwrap_or_else(PoisonError::into_inner)
} }
} }

View File

@ -16,8 +16,13 @@ use bevy_utils::{
Entry, HashMap, HashSet, Entry, HashMap, HashSet,
}; };
use naga::valid::Capabilities; use naga::valid::Capabilities;
use parking_lot::Mutex; use std::{
use std::{borrow::Cow, hash::Hash, mem, ops::Deref}; borrow::Cow,
hash::Hash,
mem,
ops::Deref,
sync::{Mutex, PoisonError},
};
use thiserror::Error; use thiserror::Error;
#[cfg(feature = "shader_format_spirv")] #[cfg(feature = "shader_format_spirv")]
use wgpu::util::make_spirv; use wgpu::util::make_spirv;
@ -587,7 +592,10 @@ impl PipelineCache {
&self, &self,
descriptor: RenderPipelineDescriptor, descriptor: RenderPipelineDescriptor,
) -> CachedRenderPipelineId { ) -> 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()); let id = CachedRenderPipelineId(self.pipelines.len() + new_pipelines.len());
new_pipelines.push(CachedPipeline { new_pipelines.push(CachedPipeline {
descriptor: PipelineDescriptor::RenderPipelineDescriptor(Box::new(descriptor)), descriptor: PipelineDescriptor::RenderPipelineDescriptor(Box::new(descriptor)),
@ -613,7 +621,10 @@ impl PipelineCache {
&self, &self,
descriptor: ComputePipelineDescriptor, descriptor: ComputePipelineDescriptor,
) -> CachedComputePipelineId { ) -> 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()); let id = CachedComputePipelineId(self.pipelines.len() + new_pipelines.len());
new_pipelines.push(CachedPipeline { new_pipelines.push(CachedPipeline {
descriptor: PipelineDescriptor::ComputePipelineDescriptor(Box::new(descriptor)), descriptor: PipelineDescriptor::ComputePipelineDescriptor(Box::new(descriptor)),
@ -773,7 +784,10 @@ impl PipelineCache {
let mut pipelines = mem::take(&mut self.pipelines); 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(..) { for new_pipeline in new_pipelines.drain(..) {
let id = pipelines.len(); let id = pipelines.len();
pipelines.push(new_pipeline); pipelines.push(new_pipeline);

View File

@ -10,7 +10,10 @@ use bevy_utils::{default, tracing::debug, HashMap, HashSet};
use bevy_window::{ use bevy_window::{
CompositeAlphaMode, PresentMode, PrimaryWindow, RawHandleWrapper, Window, WindowClosed, CompositeAlphaMode, PresentMode, PrimaryWindow, RawHandleWrapper, Window, WindowClosed,
}; };
use std::ops::{Deref, DerefMut}; use std::{
ops::{Deref, DerefMut},
sync::PoisonError,
};
use wgpu::{BufferUsages, TextureFormat, TextureUsages, TextureViewDescriptor}; use wgpu::{BufferUsages, TextureFormat, TextureUsages, TextureViewDescriptor};
pub mod screenshot; 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 // 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 // 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? // 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) { if let Some(window) = extracted_windows.get_mut(&window) {
window.screenshot_func = Some(screenshot_func); window.screenshot_func = Some(screenshot_func);
} }

View File

@ -1,4 +1,4 @@
use std::{borrow::Cow, path::Path}; use std::{borrow::Cow, path::Path, sync::PoisonError};
use bevy_app::Plugin; use bevy_app::Plugin;
use bevy_asset::{load_internal_asset, Handle}; use bevy_asset::{load_internal_asset, Handle};
@ -6,7 +6,7 @@ use bevy_ecs::prelude::*;
use bevy_log::{error, info, info_span}; use bevy_log::{error, info, info_span};
use bevy_tasks::AsyncComputeTaskPool; use bevy_tasks::AsyncComputeTaskPool;
use bevy_utils::HashMap; use bevy_utils::HashMap;
use parking_lot::Mutex; use std::sync::Mutex;
use thiserror::Error; use thiserror::Error;
use wgpu::{ use wgpu::{
CommandEncoder, Extent3d, ImageDataLayout, TextureFormat, COPY_BYTES_PER_ROW_ALIGNMENT, CommandEncoder, Extent3d, ImageDataLayout, TextureFormat, COPY_BYTES_PER_ROW_ALIGNMENT,
@ -50,6 +50,7 @@ impl ScreenshotManager {
) -> Result<(), ScreenshotAlreadyRequestedError> { ) -> Result<(), ScreenshotAlreadyRequestedError> {
self.callbacks self.callbacks
.get_mut() .get_mut()
.unwrap_or_else(PoisonError::into_inner)
.try_insert(window, Box::new(callback)) .try_insert(window, Box::new(callback))
.map(|_| ()) .map(|_| ())
.map_err(|_| ScreenshotAlreadyRequestedError) .map_err(|_| ScreenshotAlreadyRequestedError)