Add PartialEq and Eq for more types

[aloucks]

Depends on gfx-rs/wgpu#535

[grovesNL]

I think this will have the same issue as mentioned in #202

[aloucks]

@grovesNL I'm not sure I understand how adding Hash implementations to these handle types causes an issue with the web backend. I can understand that unique IDs for javascript objects could be a problem, but I don't see how they are related. Wouldn't the javascript object reference the handle, not the other way around?

[aloucks]

I'd like to get a better understanding as to why adding Hash causes a problem. Admittingly, I know very little about wasm/bindgen interop with Rust, but many of the std structs and primitives implement Hash - I don't understand why having it here is different.

In any case, I think we could ultimately get around it with something like:

#[cfg_attr(not(target_arch = "wasm32"), derive(Hash))]

[grovesNL]

Types like Adapter on the web would contain a reference to the JavaScript object (instead of an ID). For example, on the web backend we'd redefine AdapterId like

wgpu-rs/src/backend/web.rs

Line 6 in 1084bbd

pub type AdapterId = web_sys::GpuAdapter;

to reference GpuAdapter, but GpuAdapter isn't able to derive Hash because the GpuAdapter JS object is an opaque handle. We could add a layer of indirection to map ID<->JS object for the web backend but it would be nice to avoid this if possible.

[aloucks]

@grovesNL Thank you, now I understand how it's an issue. I've pushed a new commit that will conditionally implement Hash for only the non-wasm32 targets

[grovesNL]

Ok great, thanks! I'm still not sure whether it's best to hide it behind target_arch, hide it behind a feature, disallow it on both native+web, add extra indirection to the web backend (ID<->JS object map) to expose it on both native+web, or something else.

For example, it would be a bit unfortunate for portability if crates tend to rely on this, then later realize they can't easily target the web just by switching targets.

[aloucks]

You could give every object a unique ID using AtomicU64::fetch_add and implement Hash by hand.

[aloucks]

Another option is to just use a non-atomic incrementing u64 if it's strictly used for hash only (and not identity). An occasional dirty read won't be the end of the world.

Would you be comfortable to merge if I commented out the Hash derives to be sorted out later. This way we can at least get Eq and PartialEq implemented.

[grovesNL]

Yeah that sounds good to me 👍 I think these are interesting ideas and worth exploring, but it might be safer to remove the Hash derives for now and figure this out later. It should still be possible for users to manually implement their own Hash on newtypes for now if it's really useful to them.

[aloucks]

It should still be possible for users to manually implement their own Hash on newtypes for now if it's really useful to them.

Are the id fields exposed? I don't think there's any value for a user to hash, which is ironically the same problem that you have in the web space.

[grovesNL]

Right, because id isn't exposed it would basically mean using a solution like you mentioned above, or creating their own maps of handle<->object if they need handles anyway, etc.

[aloucks]

@grovesNL I've removed the Hash derives.

[grovesNL]

Thanks! It looks like CI has some build errors

[aloucks]

Thanks! It looks like CI has some build errors

Yeah, it depends the changes here: gfx-rs/wgpu#535

[grovesNL]

Oh whoops, sorry – I thought we already updated to a newer rev

[kvark]

please rebase

[kvark]

@aloucks would you want to snuck this in for 0.5 or follow-up?

[aloucks]

@kvark Yes. I'll rebase and push this evening.

[aloucks]

@kvark should be good now.

[kvark]

Thank you! Please squash as well as remove the debug leftovers :)

[kvark]

please remove

[aloucks]

@kvark - That Temp struct still exists. If I remove the PartialEq impl it will break the PartialEq impl for Device. Do you want me to remove Temp and any references to it?

[aloucks]

@kvark I've removed Temp and all references to it. As described above, I can't remove the PartialEq impl without removing the struct or it breaks PartialEq for Device. The commits are squashed now too.

[kvark]

@aloucks wow, Temp wasn't even introduced by your PR.
Sorry about bringing this up! I think I'm having an attention deficit, and this was my mistake.

Main question is: where is it useful to compare any ObjectId for equality?
I'm thinking, since you can't clone them, you have to have only one object anyway, so you'd only be comparing a reference to a reference, in which case you might as well do a pointer eq.

[aloucks]

You can't clone them but you can have multiple references to them. The original PR included Hash implementations so you could make them keys in a map or a set. I'm hoping to get that back eventually. baby steps..

Missing PartialEq and Eq also makes it impossible to drive those on structs that contain these handles.

[kvark]

You can't clone them but you can have multiple references to them.

Yes, thinking about this more, I feel stronger that these derives are not necessary. PartialEq only makes sense for things that you can have multiple instances of. If you have unique instances, and just different references, then you should be comparing the references.

struct RefKey<'a, T: 'a>(&'a T);
impl PartialEq for RefKey<'_, _> {
  fn eq(&self, other: &Self) -> bool {
    std::ptr::eq(self.0, other.0)
  }
}

Similarly can be done for Arc/Rc. In any case, this isn't needed in wgpu-rs itself.

[aloucks]

Not having PartialEq and Eq also prohibits comparing Arc<Buffer> with Arc<Buffer>.

And:

Missing PartialEq and Eq also makes it impossible to drive those on structs that contain these handles.

[aloucks]

I feel stronger that these derives are not necessary. PartialEq only makes sense for things that you can have multiple instances of.

I strongly disagree with this. It's makes comparison with Arc extremely uneconomic for no real benefit. The lack of Clone does not mean it shouldn't implement Eq.

[kvark]

Not having PartialEq and Eq also prohibits comparing Arc with Arc.

Yes, and is it any different from &Buffer? In both cases, you can just wrap an object into a unit struct like ArcKey that has a generic implementation of PartialEq, and you are good to go. This is just 5 lines of code.

Missing PartialEq and Eq also makes it impossible to drive those on structs that contain these handles.

If your struct contains Buffer by value, it means that structs is also unique, so you'd just implement PartialEq for the references of it (or pointers to it), not for the struct itself (or just use the RefKey one I posted). If you think about it, comparing by raw pointer/reference value is faster anyway, so in the end you'd want to have that regardless of whether or not we have PartialEq.

[aloucks]

In both cases, you can just wrap an object into a unit struct like ArcKey that has a generic implementation of PartialEq, and you are good to go. This is just 5 lines of code.

But how and why is this of any value? Now I need to add to also create a Deref impl for ArcKey, when Arc already has all this. There is no downside to implementing Eq.

[kvark]

Our object wrappers currently are mostly just carrying an ID, but this isn't necessarily the case. For example, Device and Texture have other data than the ID. When we get #156, we may see more difference: things like passes would now contain raw encoded data.

So in the end we are likely going to have 3 different backends, or implementations of these types (wgpu-core, wgpu-native, and web-sys). Putting any derive on them means more friction and less freedom (i.e. it will be less straightforward to extend the structs) to these implementations. The amount of code that would go into wgpu-rs itself (manual implementation of the traits for 5+ types, potentially growing) would then exceed the amount of code that an application would need to have (i.e. RefKey used for everything that it needs). Moreover, by taking the wrapper path, the implementation has more control over what it needs to compare, and the comparisons are faster (since there is no need to look at the contents of the structs at all).

So in the end, it seems to me that this task is not obviously better solved by wgpu-rs in practice. And in theory, PartialEq is not what you want to do for comparing references/smart pointers to types. Let's see if your library or application has any trouble with handling it on your side, let's hear if @mitchmindtree or @kyren share the same complaint, and adapt.

[kyren]

Just my 2c, since you pinged me:

We currently have no need for Eq or PartialEq implementations for any of the opaque, unique handle types like Buffer.

I'd rather the API be consistent, and I definitely think wgpu-rs shouldn't pick a nonzero-cost strategy to make a type Eq or PartialEq, since the user should really make that choice.

If making a unique (non-Clone) handle type Eq or PartialEq is a compatibility or maintenance hazard then that seems like a good idea not to have those impls.

Now, if there is definitely no compatibility hazard or maintenance burden then I suppose it's not harmful to have those impls, but I can't foresee using them for anything (again, just talking about the unique handle-to-resources types like Device, Buffer, etc).

[aloucks]

The web_sys objects also implement PartialEq and Eq for these types:

Example:
https://rustwasm.github.io/wasm-bindgen/api/web_sys/struct.GpuBuffer.html

[kvark]

True. It's easy to see why this is the case though: JS objects on the web in general are refcounted, so web-sys has the semantics of Arc built-in. But we do not currently have that. There were talks about doing this though, in particular because of interest from Google in exposing this in Dawn portably (webgpu-native/webgpu-headers#15).

[kvark]

I think we reached the conclusion that so far the PartialEq/Eq are not needed. Please feel free to raise this topic again for a follow-up!