Change MemoryDevice::map/unmap_memory #34
Conversation
Now those methods require &mut M e.g. externally synchronized memory object Fixes #33
| core::{ | ||
| convert::TryFrom as _, | ||
| ptr::{copy_nonoverlapping, NonNull}, | ||
| sync::atomic::{AtomicU8, Ordering::*}, | ||
| // sync::atomic::{AtomicU8, Ordering::*}, |
| @@ -158,37 +159,33 @@ impl<M> MemoryBlock<M> { | |||
| "`offset + size` is out of memory block bounds" | |||
| ); | |||
|
|
|||
| let ptr = match self.flavor { | |||
| MemoryBlockFlavor::Dedicated => { | |||
| let ptr = match &mut self.flavor { | |||
There was a problem hiding this comment.
fwiw, in gfx/wgpu code we prefer to not match on references. It's your call here, so no objections, just fyi
There was a problem hiding this comment.
And use ref and ref mut in patterns? As I understand it, the semantics is the same. Just choice of style and MSRV
There was a problem hiding this comment.
Right. My preference is to make this explicit at the match pattern side, i.e. knowing the type being matched it's easier to see what is referenced, how deep, what's moved, or copied otherwise, based on those ref.
| @@ -370,3 +318,21 @@ impl<M> MemoryBlock<M> { | |||
| self.props.contains(MemoryPropertyFlags::HOST_CACHED) | |||
| } | |||
| } | |||
|
|
|||
| fn acquire_mapping(mapped: &mut bool) -> bool { | |||
There was a problem hiding this comment.
nit: could probably be merged with release_mapping into something like switch_mapping(mapped: &mut bool, to: bool) -> bool
| "All blocks must be deallocated before cleanup" | ||
| ); | ||
| device.deallocate_memory(chunk.memory); | ||
|
|
||
| match Arc::try_unwrap(chunk.memory) { |
There was a problem hiding this comment.
it's redundant to call is_unique only to follow by try_unwrap. They do the same thing
There was a problem hiding this comment.
The intent was to panic in debug here and merely whining into traces in release
There was a problem hiding this comment.
you can still do this without is_unique, can't you?
Err(_) => {
debug_assert!(false, "All blocks must be deallocated before cleanup"),
#[cfg(feature = "tracing")]
tracing::error!("Cleanup before all memory blocks are deallocated");
}
``
gpu-alloc/src/linear.rs
Outdated
| if chunk.allocated == 0 { | ||
| drop(block); | ||
|
|
||
| if is_unique(&mut chunk.memory) { |
There was a problem hiding this comment.
you could also do Arc::get_mut here and scrap is_unique completely
There was a problem hiding this comment.
Arc::get_mut costs more
There was a problem hiding this comment.
Interesting indeed. It does the atomics on the weak count, which you don't want to, reasonably.
You'd need to make it so the debug_assert code inside is_unique doesn't run in optimized builds. Otherwise, as the PR stands, it also calls get_mut internally, anyway.
There was a problem hiding this comment.
Well, debug_assert code does not run in optimized builds by definition of debug_assert.
There was a problem hiding this comment.
Oh it runs, it just doesn't panic. That's one of the weird things I stumbled upon back in the day.
There was a problem hiding this comment.
Nope. It doesn't run, but compiles.
debug_assert!(expr) expands to if cfg!(debug_assertions) { assert!(expr); }
There was a problem hiding this comment.
hmm, indeed! I got confused here
There was a problem hiding this comment.
I was thinking about the type checks (i.e. using something that's under #[cfg(debug_assertions)] is not possible in there), not run-time, so that was my confusion. Thank you for correcting me!
1160: Update gpu-alloc and remove the associated locking r=kvark a=kvark **Connections** Takes advantage of zakarumych/gpu-alloc#34 **Description** This PR removes the locking from our gpu-alloc wrapper. **Testing** Untested Co-authored-by: Dzmitry Malyshau <[email protected]>
Now those methods require &mut M
e.g. externally synchronized memory object
Fixes #33