mtl: Use pointers for temporary state #2175
Conversation
|
How does this approach ensure safety? It seems like it would be error prone. |
|
@jrmuizel It would be nice to rely on Objective-C to reference count these correctly, but we shouldn't really need it. The objects we expose externally have well-defined lifetimes matching the Vulkan spec. That's why we could decide to use the We only have to be really careful while handling temporary objects that we create internally. We already have to work around this issue in some other places, for example we have |
|
I'm going to think about this some more and try to restrict pointer usage to high frequency retains/releases as @kvark mentioned on Gitter |
| @@ -938,11 +921,12 @@ fn exec_render<'a>(encoder: &metal::RenderCommandEncoderRef, command: soft::Rend | |||
| encoder.set_stencil_front_back_reference_value(front, back); | |||
| } | |||
| Cmd::BindBuffer { stage, index, buffer, offset } => { | |||
| let native = buffer.as_ref().map(|b| b.as_native()); | |||
There was a problem hiding this comment.
@kvark without as_ref, the borrow checker complains because b is dropped in these cases
grovesNL
changed the title
|
Updated the commit – slightly safer now with less pointer usage for low frequency calls |
src/backend/metal/src/internal.rs
Outdated
| @@ -71,8 +76,8 @@ impl Channel { | |||
|
|
|||
|
|
|||
| pub struct SamplerStates { | |||
| nearest: metal::SamplerState, | |||
| linear: metal::SamplerState, | |||
| nearest: SamplerPtr, | |||
There was a problem hiding this comment.
I don't think we should have weak pointers here (in this module)
| @@ -260,7 +261,7 @@ impl hal::DescriptorPool<Backend> for DescriptorPool { | |||
| sampler_offset += layout.count; | |||
| slice | |||
| .iter() | |||
| .map(|s| Some(s.clone())) | |||
| .map(|s| Some(SamplerPtr(s.as_ptr()))) | |||
There was a problem hiding this comment.
isn't this just a copy? e.g. ca't we just do cloned() instead of map
There was a problem hiding this comment.
It's still DescriptorSetLayout::Emulated(Vec<pso::DescriptorSetLayoutBinding>, Vec<metal::SamplerState>). Do we want Vec<SamplerPtr> here instead?
There was a problem hiding this comment.
I assume we do want SamplerPtr, but even then we still need to map to get Some here
There was a problem hiding this comment.
oh, nvm then. I thought it's SamplerPtr, but it doesn't have to be
There was a problem hiding this comment.
Let's leave metal::SamplerState for now I guess – we appear to rely on it being retained here (not sure if its intentional or if there is supposed to be another owner)
|
@kvark thanks, updated to remove |
2175: mtl: Use pointers for temporary state r=kvark a=grovesNL Partially fix to #2161 PR checklist: - [ ] `make` succeeds (on *nix) - [ ] `make reftests` succeeds - [ ] tested examples with the following backends: This replaces buffer/texture/sampler object usage with pointers instead. This allows us to bypass Objective-C's reference counting (i.e. retain/release). The idea is to create the same objects as before, but purposely leak them and acquire a pointer to them instead. When we're finished with the objects, we take back ownership and release as usual. We should be able to do this either through `drop` on the owning struct, or through relevant `destroy_*` functions. Still heavily WIP (but works with quad/Dota 2) because there are a few places that will cause leaks or use-after-free in its current state (need to fix cases where the same image is returned for an image view, verify `drop` exists on all the correct owners, etc.) Co-authored-by: Joshua Groves <[email protected]>
Partially fix to #2161
PR checklist:
makesucceeds (on *nix)make reftestssucceedsThis replaces buffer/texture/sampler object usage with pointers instead. This allows us to bypass Objective-C's reference counting (i.e. retain/release). The idea is to create the same objects as before, but purposely leak them and acquire a pointer to them instead. When we're finished with the objects, we take back ownership and release as usual. We should be able to do this either through
dropon the owning struct, or through relevantdestroy_*functions.Still heavily WIP (but works with quad/Dota 2) because there are a few places that will cause leaks or use-after-free in its current state (need to fix cases where the same image is returned for an image view, verify
dropexists on all the correct owners, etc.)