Make wgpu-hal resources copyable (Metal and GL only)

[kvark]

Checklist

• Run cargo clippy.
• Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
• Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Closes #3195

Description
See #3195

Testing
Examples.

[codecov-commenter]

Codecov Report

Merging #3199 (eb7dd91) into master (d536613) will decrease coverage by 0.03%.
The diff coverage is 5.55%.

@@            Coverage Diff             @@
##           master    #3199      +/-   ##
==========================================
- Coverage   64.66%   64.62%   -0.04%     
==========================================
  Files          81       81              
  Lines       38956    38976      +20     
==========================================
  Hits        25190    25190              
- Misses      13766    13786      +20     

Impacted Files	Coverage Δ
wgpu-hal/src/gles/queue.rs	60.61% <0.00%> (-0.71%)	⬇️
wgpu-hal/src/gles/device.rs	79.70% <12.50%> (-0.38%)	⬇️
wgpu-hal/src/gles/mod.rs	61.53% <50.00%> (ø)
wgpu-core/src/hub.rs	60.67% <0.00%> (-0.16%)	⬇️
wgpu-hal/src/gles/egl.rs	37.82% <0.00%> (-0.13%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[kvark]

Based on #3195 (comment) it may be the case that the idea to make them copyable is wrong to start with. But the code here is very useful to merge nevertheless.

[jimblandy]

This is on my list to look at on Wednesday.

[kvark]

@jimblandy wow, much conflicts. Is this clear to go otherwise, or we have concerns?

[jimblandy]

@kvark I have concerns about not using the owning types. I'm having a hard time persuading myself that the changes in retains and releases are balanced. I'd like to give this a final look-over tomorrow.

If you have time to add some comments about ownership and internal retention in the mean time, that'd be great.

[jimblandy]

Got some time to look at this today.

[jimblandy]

Since we call AsNative::own_from here and in texture_from_raw, shouldn't destroy_texture call AsNative::destroy?

[kvark]

Definitely. I just missed this, thanks for spotting!

[jimblandy]

Rebased.

[jimblandy]

@kvark I'm not convinced that making the resource types Copy is a good idea. It seems like you usually want an error message if you accidentally copy something like that; we don't want to make it a silent failure.

In the Metal case, we've already got idiomatic integration of Rust clone/drop behavior into Objective-C retain/release. Removing that just seems like a step backwards for no good reason. It's true, using metal_rs's foreign_obj_type wrappers generates extra reference count traffic around the surface textures, since the drawable already holds an owning reference. But is that actually a problem?

And if my concern here is correct, then I think it's beyond debate that this is error-prone.

Summary:

• I'd definitely like to take the externally_owned / DropGuard changes. That seems like a good cleanup.
• I don't think we should take the other Copy changes.

[grovesNL]

@jimblandy

It's true, using metal_rs's foreign_obj_type wrappers generates extra reference count traffic around the surface textures, since the drawable already holds an owning reference. But is that actually a problem?

For what it's worth, in general we've seen retain/release show up on profiles in real use cases because of this, especially back when we were running Dota 2 on top of gfx-portability. Switching to raw pointers for some temporary resources helped significantly there (gfx-rs/gfx#2175).

Based on that experience, it might be worth avoiding extra refcounting on the Metal side for some resources if we already track lifetimes internally (either manually or through other containers like Arc). It does tend to show up on profilers, but spread all over the place so it's one of those thousand-paper-cuts kind of performance issues that can be a bit subtle (not very noticeable with timeline performance profiles for example).

[jimblandy]

I can certainly believe that refcounting shows up in profiles in general. But except for the SurfaceTexture case, this PR should have no effect on the number of refcount operations. It just trades uses of foreign_obj_type wrappers (that is, Clone/Drop-based refcounting) in structs with manual calls to AsNative::own_from in the constructors and AsNative::destroy in the destroy functions of those structs.

And in the case of the Surface::acquire_texture, that's only called once per frame, right? That's not going to be hot.

gfx-rs/gfx#2175 seems apt: there, @jrmuizel said it would be error prone (indeed), and you said you would look into restricting pointer usage to high-frequency cases. So our values haven't changed here.

[grovesNL]

@jimblandy agreed, I just wanted to add some context in case you (or anyone else) might be curious why we avoid extra Objective-C refcounting in some other places

[jrmuizel]

@grovesNL was the reference counting performance problem happening when doing doing msg_send[retain/release] or objc_retain/objc_release?

[grovesNL]

@jrmuizel I think we only ever tried it with msg_send since we weren't totally sure about the implications of using objc_retain/objc_release directly. It's possible that would've also helped a lot though. Although even with objc_retain/objc_release it might be useful to avoid Objective-C refcounting where the lifetime is already known on the Rust side, especially if we're already wrapping the resource into our own Arcs/tracking some aggregate containing the resource at a different level.

[jimblandy]

@jrmuizel @grovesNL This is interesting but off-topic. It would be good to capture this discussion in an issue filed against metal-rs.

[kvark]

@jimblandy thanks for looking!

And if my concern #3199 (comment) is correct, then I think it's beyond debate that this is error-prone.

It's clear that wgpu-hal internals require good care and are generally easy to screw up. This applies to many places. Having a proper texture destructor is one of the easier ones. The desire to make this particular bit in the metal backend a tiny bit safer inside has a very marginal effect on wgpu-hal as a whole. On the other hand, making Metal's objects to do manual refcounting would be more consistent with the rest of the code base:

• d3d12 backend manually destroys
• vulkan backend manually releases objects
• commands in Metal backend itself use raw objc objects without refcounting

As for the user of wgpu-hal - there is a million ways to screw up object lifetime even if the object is not Copy, and all of the API is unsafe still. For example, you record a command buffer and deleted a buffer used in a command. So non-Copy has a very small benefit in terms of safety, but Copy would make a real benefit in terms of wgpu-hal usability by other parties, including wgpu-core itself (and potentially things like WebRender?).

I'm describing both inside wgpu-hal and external factors because I'm not sure which one you are most concerned with :)
In any case, none of my experimental work depends on this change right now, so I'm not blocked, and I don't have strong feelings about this change. I just think it's a small win.

[jimblandy]

It's certainly true that using a ref-counting type here does not magically make everything else that hal does safe.

Copy would make a real benefit in terms of wgpu-hal usability by other parties, including wgpu-core itself (and potentially things like WebRender?).

This is the part I don't understand: what is the upside? Two questions:

1. The wgpu_hal::Api associated types for resources are not Copy at the moment, and cannot become so unless all backends can manage it. So wgpu-hal's users can't benefit from this at all. Is the plan to somehow manage to make all the backends' resource types Copy, and then include that as a constraint on Api's associated types?

2. What is the advantage of Copy? The ability to silently copy values makes writing code marginally easier, at the cost of making tracking ownership harder. Having a type require explicit clone calls is an advantage.

In short, I think you've got some plans, or some experiences, behind this change that I'm not seeing. Without understanding the advantages, I don't want to take the change.

[jimblandy]

Let's get the DropGuard / externally-owned changes merged separately, and then we can work out Copy without that entanglement.

[jimblandy]

You know, maybe there's an easy compromise here: how about just making them Clone, but not Copy? What I'm concerned about is inadvertent duplication, and Clone avoids that problem.

[cwfitzgerald]

Due to serious safety concerns (especially after a vulnerability directly related to copyable handles), I don't think we can land this in its current form.