Implement PartialEq for JS all the things

[ZacLiveEarth]

Motivation

I have, eg: a js_sys::JsString or a js_sys::Array and would like to put them into a struct with #[derive(PartialEq)]

Proposed Solution

Wrappers over js values would implement PartialEq by first comparing the heap pointer for equality and if unequal fall back to calling a javascript function which returns a strict equality comparison (Special consideration may need to be made for floats and NaN in particular to not use the fast-out since NaN != NaN)

Alternatives

One could manually implement equality for the struct without the use of derive, as well as a wrapper over strict equality for js-sys and other wasm-bindgen bindings... or they could use the newtype pattern. Aside from a lot of potential boilerplate they would need to access the internals of a binding (eg: it's heap pointer) to implement the fast out. When a part of a larger solution involving traits or multiple libraries these solutions may fall apart.

[Pauan]

Wrappers over js values would implement PartialEq by first comparing the heap pointer for equality

I assume this is suggested for the sake of performance. I'm not sure if it would actually be faster in practice, but I guess we can find out.

For the actual equality test, we can use Object::is, which handles all the sticky nuances correctly (though it does distinguish -0 and +0, which may not be desired).

On the other hand, Rust distinguishes between -0 and +0, and there are some places in JS which also distinguish them, so maybe it's completely fine.

(I should also note that in Rust NAN is not equal to itself, which is consistent with === in JS. Personally, I think that's probably a mistake, but I'm not an expert on such matters.)

[ZacLiveEarth]

It should be noted that Object.is and strict equality === are not quite the same. In my limited experience, I would guess most developers to expect strict equality for PartialEq as it is more widely used in js code.

I was thinking of something like this in the generated js file...

export function __wbindgen_strict_equality(arg0, arg1) { return getObject(arg0) === getObject(arg1) }

You are correct that I had suggested the heap pointer comparison for performance to avoid the trampoline. Whether it would be better would depend on how many times it hit, and the cost of the trampoline.... both of which are pretty situational.

[Pauan]

It should be noted that Object.is and strict equality === are not quite the same.

Indeed I am aware of that. I think Object.is is the correct choice.

You are correct that I had suggested the heap pointer comparison for performance to avoid the trampoline.

What trampoline are you referring to?

[ZacLiveEarth]

I was borrowing the word trampoline from this article, but thunking probably would have been a better choice. What I meant was the cost of swapping between wasm and js, which with wasm-bindgen infers grabbing something out of an array... To my understanding this means inferring multiple possible cache misses when a simple integer comparison may have done the trick instead.

[alexcrichton]

I think I would agree that all types in js_sys (and probably web_sys eventually) should implement PartialEq, and I think we can just do that with #[derive(PartialEq)] on all the annotations (they already have Debug and whatnot)

Currently that'd delegate to PartialEq for JsValue which uses === instead of Object.is as @Pauan recommends, but we should be able to easily switch that! I suspect that this doesn't matter for types in js_sys because it sounds like Object.is and === behave the same for object-like things, but it'd be a behavior change for JsValue. Alternatively we could just manually implement PartialEq for Object to use Object.is and basically everything would delegate to that through #[derive]

[Pauan]

@ZacLiveEarth I was borrowing the word trampoline from this article

Oh, okay, but those wrappers don't exist (since they've been optimized out already).

And using Object.is will probably be faster than === (because it can call directly to the host function, without going through a JS function at all).

Also, the wasm-bindgen heap will be going away (it will be replaced with a wasm table and anyref).

So I wouldn't worry about any of that right now. That seems like premature optimization to me.

We can always add in that optimization later in a backwards compatible way (after doing benchmarks, of course).

---

@alexcrichton One (potential) benefit of Object.is is that we could then impl Eq in addition to PartialEq.

So I would be wary about using ===, at least until we've fully decided on what we want to do.

So your plan of impling it for Object and not JsValue sounds good to me, since in that case Object.is and === have the same semantics.

[alexcrichton]

Excellent points @Pauan! I've posted an idea of what this might look like at #1444

[ZacLiveEarth]

That seems like premature optimization to me.

I work on a system that animates and renders 200,000+ entities every 16ms while concurrently streaming up to 750,000 updates to the world state per second. Cache misses, lock-free threading and the like are my life. At present, I'm porting that system to web... which isn't quite at that level yet. When people say 'premature optimization', I feel these words are often used to shut down legitimate conversations. The hard part is that you never know the needs of the person raising the concern. Sure, 9 times out of 10 premature optimization is a real problem. But, wasm-bindgen is a library that is for people using Rust and WebAssembly... I'm sure some of the users care of this library care about it's performance. I do appreciate that anyref is going to alleviate this problem, so that this problem is a temporary one and temporary problems should get less attention to avoid wasted effort. Also, I don't want to say for sure whether the fast-out is a worthwhile optimization, but I do want wasm-bindgen to care about performance and consider it at every step.

[Pauan]

When people say 'premature optimization', I feel these words are often used to shut down legitimate conversations.

I certainly wasn't trying to do that!

I merely said that the situation is currently in flux (and is changing rapidly, and will change more in the future), so any performance improvements made now might be invalidated soon.

In addition, all optimizations (whether premature or not) must be backed by evidence (such as benchmarks, ideally of a real program), because otherwise it might make performance worse. I hope that is not a controversial viewpoint.

I thought I made it pretty clear that I wasn't against the idea, I just think we need to do it later (with accompanying benchmarks). Sorry I wasn't clear enough.

Believe me, this is Rust, we care a lot about performance (and I personally care a lot about performance, which is one reason why I chose Rust over other languages).

And wasm-bindgen has been carefully designed so that it can be as performant as it can be right now, and will become more performant in the future (with host bindings). Our plan is to have faster-than-JS performance.

There might be some small tweaks we can do to edge out a bit more performance, so if you find anything like that, let us know (with accompanying use cases and benchmarks).

[ZacLiveEarth]

Thanks @Pauan, I appreciate that response a lot.

[alexcrichton]

Done in #1444!