qunit icon indicating copy to clipboard operation
qunit copied to clipboard

Core: Optimize strictEquality checks

Open izelnakri opened this issue 2 years ago • 1 comments

Removes extra if checks, variable mutations and .valueOf() calls from values that dont need it.

After seeing the test results, it seems to be this is an intentional "feature" rather than a bug. What would be the consequences if we remove this feature for v3? Are there really a lot of libraries/apps that depend on this feature?

izelnakri avatar Jul 06 '22 01:07 izelnakri

The inline comment might be confusing on its own. This code isn't there for the object type, but rather the the boolean/number/string type, and this type is determined not based on typeof but based on the Object toString tag.

x = new Number(2); 
Object.prototype.toString.call(x);
//> "[object Number]"

This returns that way for both primitives and boxed primitives, and lets the code enter there, and for objects we unbox them in the deepEqual code path where we allow similar objects that are value-wise equivalent to be considered equal. And as boxed primitives generally have no properties, we compare their internal value this way.

I think we could handle this in a more optimal way, e.g. switch to typeof perhaps for most things so that we can have the common code path for actual primitives and unrelated objects be simple and fast. And then perhaps put boxed primitives in a different bucket that we only computing in objectType for when typeof is object, and then e.g. put it in something we use internally only like boxed or some such.

For QUnit 3 I'd like to keep the list of removals fairly short and mostly limited to structural changes, not e.g. actual assertion behaviour. But, I agree this is esoteric and if the above ends up negating the optimisation you're looking for or otherwise more complex than we like, we could reconsider. Using the result of typeof for more cases might speed things up a bit as well.

Krinkle avatar Jul 06 '22 01:07 Krinkle

I implemented something similar to what we described here in f578596345f8ab5a019bf1e593555ed6bf62a05a. I approached it a bit differently after I realized we could split things typeof vs objectType thus removing the ambiguity. The cases where we know the value is a primitive string, it now always uses strict equality. The cases where one or both are a boxed primitive (e.g. String object), are now handled by the "object" callback, and I moved the valueOf handling to there to maintain support for it.

Boldly closing as such. Don't hestitate to comment if I missed something. More PRs always welcome as well!

Krinkle avatar Oct 18 '22 01:10 Krinkle