feat: switch away from parking_lot::Mutex

Switching away from `parking_lot::Mutex` as `std::sync::Mutex` performs
better in all scenarios on both of our targets (linux/windows).
This commit is contained in:
zack 2025-03-30 16:27:35 -04:00
parent 234e2ab78d
commit f71f0d8e10
No known key found for this signature in database
GPG key ID: EE8A2B709E2401D1
10 changed files with 66 additions and 38 deletions

3
Cargo.lock generated
View file

@ -613,7 +613,6 @@ version = "0.1.0"
dependencies = [ dependencies = [
"ash", "ash",
"ash-window", "ash-window",
"parking_lot",
"thiserror 2.0.12", "thiserror 2.0.12",
"tracing", "tracing",
"winit", "winit",
@ -1308,7 +1307,6 @@ dependencies = [
"gfx_hal", "gfx_hal",
"glam", "glam",
"gpu-allocator", "gpu-allocator",
"parking_lot",
"resource_manager", "resource_manager",
"shaderc", "shaderc",
"thiserror 2.0.12", "thiserror 2.0.12",
@ -1324,7 +1322,6 @@ dependencies = [
"ash", "ash",
"gfx_hal", "gfx_hal",
"gpu-allocator", "gpu-allocator",
"parking_lot",
"thiserror 2.0.12", "thiserror 2.0.12",
"tracing", "tracing",
] ]

View file

@ -27,7 +27,6 @@ egui = "0.31"
bytemuck = { version = "1.21.0", features = ["derive"] } bytemuck = { version = "1.21.0", features = ["derive"] }
tracing = "0.1" tracing = "0.1"
tracing-subscriber = { version = "0.3", features = ["json"] } tracing-subscriber = { version = "0.3", features = ["json"] }
parking_lot = "0.12.3"
thiserror = "2.0.12" thiserror = "2.0.12"

View file

@ -9,4 +9,3 @@ ash-window.workspace = true
thiserror.workspace = true thiserror.workspace = true
tracing.workspace = true tracing.workspace = true
winit.workspace = true winit.workspace = true
parking_lot.workspace = true

View file

@ -1,9 +1,10 @@
use ash::vk; use ash::vk;
use parking_lot::Mutex;
use std::collections::HashSet; use std::collections::HashSet;
use std::ffi::CStr; use std::ffi::CStr;
use std::sync::Weak; use std::{
use std::{collections::HashMap, sync::Arc}; collections::HashMap,
sync::{Arc, Mutex},
};
use crate::error::{GfxHalError, Result}; use crate::error::{GfxHalError, Result};
use crate::instance::Instance; use crate::instance::Instance;
@ -146,7 +147,7 @@ impl Device {
// Lock the mutex and insert the created queues into the map within the Arc<Device> // Lock the mutex and insert the created queues into the map within the Arc<Device>
{ {
// Scope for the mutex guard // Scope for the mutex guard
let mut queues_map_guard = device_arc.queues.lock(); let mut queues_map_guard = device_arc.queues.lock()?;
*queues_map_guard = queues_to_insert; // Replace the empty map with the populated one *queues_map_guard = queues_to_insert; // Replace the empty map with the populated one
tracing::debug!( tracing::debug!(
"Device Arc populated with {} queues (Stage 2).", "Device Arc populated with {} queues (Stage 2).",
@ -185,15 +186,21 @@ impl Device {
/// Gets a wrapped queue handle. /// Gets a wrapped queue handle.
/// Currently only supports queue index 0 for each family. /// Currently only supports queue index 0 for each family.
pub fn get_queue(&self, family_index: u32, queue_index: u32) -> Option<Arc<Queue>> { pub fn get_queue(&self, family_index: u32, queue_index: u32) -> Result<Arc<Queue>> {
if queue_index != 0 { if queue_index != 0 {
tracing::warn!("get_queue currently only supports queue_index 0"); tracing::warn!("get_queue currently only supports queue_index 0");
return None; return Err(GfxHalError::MissingQueueFamily(
"get_queue only supports queue_index 0".to_string(),
));
} }
self.queues self.queues
.lock() .lock()?
.get(&(family_index, queue_index)) .get(&(family_index, queue_index))
.cloned() .cloned()
.ok_or(GfxHalError::MissingQueueFamily(
"could not get queue family".to_string(),
))
} }
/// Gets the primary graphics queue (family index from `graphics_queue_family_index`, queue index 0). /// Gets the primary graphics queue (family index from `graphics_queue_family_index`, queue index 0).

View file

@ -56,9 +56,19 @@ pub enum GfxHalError {
#[error("Error loading the ash entry.")] #[error("Error loading the ash entry.")]
AshEntryError(#[from] ash::LoadingError), AshEntryError(#[from] ash::LoadingError),
/// Poisoned Mutex
#[error("Error from poisoned mutex: {0}")]
MutexPoisoned(String),
/// Placeholder for other specific errors. /// Placeholder for other specific errors.
#[error("An unexpected error occurred: {0}")] #[error("An unexpected error occurred: {0}")]
Other(String), Other(String),
} }
pub type Result<T, E = GfxHalError> = std::result::Result<T, E>; pub type Result<T, E = GfxHalError> = std::result::Result<T, E>;
impl<T> From<std::sync::PoisonError<T>> for GfxHalError {
fn from(e: std::sync::PoisonError<T>) -> Self {
Self::MutexPoisoned(e.to_string())
}
}

View file

@ -1,7 +1,6 @@
use std::sync::Arc; use std::sync::{Arc, Mutex};
use ash::{vk, Device as AshDevice}; use ash::{vk, Device as AshDevice};
use parking_lot::Mutex;
use crate::device::Device; use crate::device::Device;
use crate::error::Result; use crate::error::Result;

View file

@ -13,7 +13,6 @@ gpu-allocator.workspace = true
egui.workspace = true egui.workspace = true
egui-ash-renderer.workspace = true egui-ash-renderer.workspace = true
winit.workspace = true winit.workspace = true
parking_lot.workspace = true
gfx_hal = { path = "../gfx_hal" } gfx_hal = { path = "../gfx_hal" }
resource_manager = { path = "../resource_manager" } resource_manager = { path = "../resource_manager" }

View file

@ -1,12 +1,15 @@
use std::{ffi::CStr, sync::Arc}; use std::{
ffi::CStr,
sync::{Arc, Mutex},
};
use ash::vk; use ash::vk;
use egui_ash_renderer::{DynamicRendering, Options, Renderer as EguiRenderer};
use gfx_hal::{ use gfx_hal::{
device::Device, error::GfxHalError, queue::Queue, surface::Surface, swapchain::Swapchain, device::Device, error::GfxHalError, queue::Queue, surface::Surface, swapchain::Swapchain,
swapchain::SwapchainConfig, sync::Fence, sync::Semaphore, swapchain::SwapchainConfig, sync::Fence, sync::Semaphore,
}; };
use gpu_allocator::{vulkan::Allocator, MemoryLocation}; use gpu_allocator::{vulkan::Allocator, MemoryLocation};
use parking_lot::Mutex;
use resource_manager::{ImageHandle, ResourceManager, ResourceManagerError}; use resource_manager::{ImageHandle, ResourceManager, ResourceManagerError};
use thiserror::Error; use thiserror::Error;
use tracing::{debug, error, info, warn}; use tracing::{debug, error, info, warn};
@ -79,6 +82,8 @@ pub struct Renderer {
swapchain_format: vk::SurfaceFormatKHR, swapchain_format: vk::SurfaceFormatKHR,
swapchain_extent: vk::Extent2D, swapchain_extent: vk::Extent2D,
egui_renderer: EguiRenderer,
depth_image_handle: ImageHandle, depth_image_handle: ImageHandle,
depth_image_view: vk::ImageView, // Store the view directly depth_image_view: vk::ImageView, // Store the view directly
depth_format: vk::Format, depth_format: vk::Format,
@ -128,10 +133,21 @@ impl Renderer {
info!("Renderer initialized successfully."); info!("Renderer initialized successfully.");
let egui_renderer = EguiRenderer::with_gpu_allocator(
resource_manager.allocator(),
device.raw().clone(),
DynamicRendering {
color_attachment_format: swapchain.format().format,
depth_attachment_format: Some(depth_format),
},
Options::default(),
)?;
Ok(Self { Ok(Self {
device, device,
graphics_queue, graphics_queue,
resource_manager, resource_manager,
egui_renderer,
allocator, // Store the allocator Arc allocator, // Store the allocator Arc
surface, surface,
swapchain: Some(swapchain), swapchain: Some(swapchain),
@ -465,6 +481,10 @@ impl Renderer {
self.depth_image_handle = new_depth_handle; self.depth_image_handle = new_depth_handle;
self.depth_image_view = new_depth_view; self.depth_image_view = new_depth_view;
// 4. Update Egui Renderer (if necessary, depends on its implementation)
// It might need the new extent or recreate internal resources.
// Assuming it handles extent changes via update_screen_descriptor called earlier.
info!( info!(
"Swapchain recreated successfully ({}x{}).", "Swapchain recreated successfully ({}x{}).",
new_extent.width, new_extent.height new_extent.width, new_extent.height

View file

@ -7,7 +7,6 @@ edition = "2021"
ash.workspace = true ash.workspace = true
gpu-allocator.workspace = true gpu-allocator.workspace = true
thiserror.workspace = true thiserror.workspace = true
parking_lot.workspace = true
tracing.workspace = true tracing.workspace = true
gfx_hal = { path = "../gfx_hal" } gfx_hal = { path = "../gfx_hal" }

View file

@ -5,7 +5,7 @@ use std::{
hash::Hash, hash::Hash,
sync::{ sync::{
atomic::{AtomicU64, Ordering}, atomic::{AtomicU64, Ordering},
Arc, Arc, Mutex,
}, },
}; };
@ -18,7 +18,6 @@ use gpu_allocator::{
vulkan::{Allocation, AllocationCreateDesc, Allocator, AllocatorCreateDesc}, vulkan::{Allocation, AllocationCreateDesc, Allocator, AllocatorCreateDesc},
MemoryLocation, MemoryLocation,
}; };
use parking_lot::Mutex;
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct BufferHandle(u64); pub struct BufferHandle(u64);
@ -63,7 +62,7 @@ impl Drop for InternalBufferInfo {
fn drop(&mut self) { fn drop(&mut self) {
trace!("Dropping InternalBufferInfo for handle: {:?}", self.handle); trace!("Dropping InternalBufferInfo for handle: {:?}", self.handle);
if let Some(allocation) = self.allocation.take() { if let Some(allocation) = self.allocation.take() {
let mut allc = self.allocator.lock(); let mut allc = self.allocator.lock().expect("to acquire mutex lock");
if let Err(e) = allc.free(allocation) { if let Err(e) = allc.free(allocation) {
error!( error!(
"Failed to free allocation for buffer handle {:?}, {}", "Failed to free allocation for buffer handle {:?}, {}",
@ -102,7 +101,7 @@ impl Drop for InternalImageInfo {
} }
// Then free memory // Then free memory
if let Some(allocation) = self.allocation.take() { if let Some(allocation) = self.allocation.take() {
let mut allocator = self.allocator.lock(); let mut allocator = self.allocator.lock().expect("to acquire mutex lock");
if let Err(e) = allocator.free(allocation) { if let Err(e) = allocator.free(allocation) {
error!( error!(
"Failed to free allocation for image handle {:?}: {}", "Failed to free allocation for image handle {:?}: {}",
@ -171,7 +170,7 @@ impl ResourceManager {
/// Gets or initializes the TransferSetup resources. /// Gets or initializes the TransferSetup resources.
fn get_transfer_setup(&self) -> Result<TransferSetup> { fn get_transfer_setup(&self) -> Result<TransferSetup> {
let mut setup_guard = self.transfer_setup.lock(); let mut setup_guard = self.transfer_setup.lock()?;
if let Some(setup) = setup_guard.as_ref() { if let Some(setup) = setup_guard.as_ref() {
// Simple check: Reset fence before reusing // Simple check: Reset fence before reusing
@ -192,10 +191,7 @@ impl ResourceManager {
.or(self.device.compute_queue_family_index()) // Try compute as fallback .or(self.device.compute_queue_family_index()) // Try compute as fallback
.unwrap_or(self.device.graphics_queue_family_index()); // Graphics as last resort .unwrap_or(self.device.graphics_queue_family_index()); // Graphics as last resort
let queue = self let queue = self.device.get_queue(queue_family_index, 0)?;
.device
.get_queue(queue_family_index, 0)
.ok_or(ResourceManagerError::NoTransferQueue)?;
// Create command pool for transfer commands // Create command pool for transfer commands
let pool_info = vk::CommandPoolCreateInfo::default() let pool_info = vk::CommandPoolCreateInfo::default()
@ -306,7 +302,7 @@ impl ResourceManager {
let requirements = unsafe { self.device.raw().get_buffer_memory_requirements(buffer) }; let requirements = unsafe { self.device.raw().get_buffer_memory_requirements(buffer) };
let allocation = self.allocator.lock().allocate(&AllocationCreateDesc { let allocation = self.allocator.lock()?.allocate(&AllocationCreateDesc {
name: &format!("buffer_usage_{:?}_loc_{:?}", usage, location), name: &format!("buffer_usage_{:?}_loc_{:?}", usage, location),
requirements, requirements,
location, location,
@ -342,7 +338,7 @@ impl ResourceManager {
handle, handle,
}; };
self.buffers.lock().insert(id, internal_info); self.buffers.lock()?.insert(id, internal_info);
debug!("Buffer created successfully: handle={:?}", handle); debug!("Buffer created successfully: handle={:?}", handle);
Ok(handle) Ok(handle)
} }
@ -441,7 +437,7 @@ impl ResourceManager {
let requirements = unsafe { self.device.raw().get_image_memory_requirements(image) }; let requirements = unsafe { self.device.raw().get_image_memory_requirements(image) };
let allocation = self.allocator.lock().allocate(&AllocationCreateDesc { let allocation = self.allocator.lock()?.allocate(&AllocationCreateDesc {
name: &format!( name: &format!(
"image_fmt_{:?}_usage_{:?}", "image_fmt_{:?}_usage_{:?}",
create_info.format, create_info.usage create_info.format, create_info.usage
@ -491,7 +487,7 @@ impl ResourceManager {
handle, handle,
}; };
self.images.lock().insert(id, internal_info); self.images.lock()?.insert(id, internal_info);
debug!("Image created successfully: handle={:?}", handle); debug!("Image created successfully: handle={:?}", handle);
Ok(handle) Ok(handle)
} }
@ -501,7 +497,7 @@ impl ResourceManager {
/// Destroys a buffer and frees its memory. /// Destroys a buffer and frees its memory.
pub fn destroy_buffer(&self, handle: BufferHandle) -> Result<()> { pub fn destroy_buffer(&self, handle: BufferHandle) -> Result<()> {
debug!("Requesting destroy for buffer handle {:?}", handle); debug!("Requesting destroy for buffer handle {:?}", handle);
let mut buffers_map = self.buffers.lock(); let mut buffers_map = self.buffers.lock()?;
// Remove the entry. The Drop impl of InternalBufferInfo handles the cleanup. // Remove the entry. The Drop impl of InternalBufferInfo handles the cleanup.
if buffers_map.remove(&handle.0).is_some() { if buffers_map.remove(&handle.0).is_some() {
debug!("Buffer handle {:?} removed for destruction.", handle); debug!("Buffer handle {:?} removed for destruction.", handle);
@ -518,7 +514,7 @@ impl ResourceManager {
/// Destroys an image, its view, and frees its memory. /// Destroys an image, its view, and frees its memory.
pub fn destroy_image(&self, handle: ImageHandle) -> Result<()> { pub fn destroy_image(&self, handle: ImageHandle) -> Result<()> {
debug!("Requesting destroy for image handle {:?}", handle); debug!("Requesting destroy for image handle {:?}", handle);
let mut images_map = self.images.lock(); let mut images_map = self.images.lock()?;
// Remove the entry. The Drop impl of InternalImageInfo handles the cleanup. // Remove the entry. The Drop impl of InternalImageInfo handles the cleanup.
if images_map.remove(&handle.0).is_some() { if images_map.remove(&handle.0).is_some() {
debug!("Image handle {:?} removed for destruction.", handle); debug!("Image handle {:?} removed for destruction.", handle);
@ -534,7 +530,7 @@ impl ResourceManager {
/// Gets non-owning information about a buffer. /// Gets non-owning information about a buffer.
pub fn get_buffer_info(&self, handle: BufferHandle) -> Result<BufferInfo> { pub fn get_buffer_info(&self, handle: BufferHandle) -> Result<BufferInfo> {
let buffers_map = self.buffers.lock(); let buffers_map = self.buffers.lock()?;
buffers_map buffers_map
.get(&handle.0) .get(&handle.0)
.map(|internal| BufferInfo { .map(|internal| BufferInfo {
@ -549,7 +545,7 @@ impl ResourceManager {
/// Gets non-owning information about an image. /// Gets non-owning information about an image.
pub fn get_image_info(&self, handle: ImageHandle) -> Result<ImageInfo> { pub fn get_image_info(&self, handle: ImageHandle) -> Result<ImageInfo> {
let images_map = self.images.lock(); let images_map = self.images.lock()?;
images_map images_map
.get(&handle.0) .get(&handle.0)
.map(|internal| ImageInfo { .map(|internal| ImageInfo {
@ -586,15 +582,18 @@ impl Drop for ResourceManager {
// Clear resource maps. This triggers the Drop impl for each Internal*Info, // Clear resource maps. This triggers the Drop impl for each Internal*Info,
// which frees allocations and destroys Vulkan objects. // which frees allocations and destroys Vulkan objects.
let mut buffers_map = self.buffers.lock(); let mut buffers_map = self.buffers.lock().expect("mutex to not be poisoned");
debug!("Clearing {} buffer entries...", buffers_map.len()); debug!("Clearing {} buffer entries...", buffers_map.len());
buffers_map.clear(); buffers_map.clear();
let mut images_map = self.images.lock(); let mut images_map = self.images.lock().expect("mutex to not be poisoned");
debug!("Clearing {} image entries...", images_map.len()); debug!("Clearing {} image entries...", images_map.len());
images_map.clear(); images_map.clear();
// Destroy transfer setup resources // Destroy transfer setup resources
let mut setup_guard = self.transfer_setup.lock(); let mut setup_guard = self
.transfer_setup
.lock()
.expect("mutex to not be poisoned");
if let Some(setup) = setup_guard.take() { if let Some(setup) = setup_guard.take() {
// take() removes it from the Option // take() removes it from the Option
debug!("Destroying TransferSetup resources..."); debug!("Destroying TransferSetup resources...");