fix(assert): check property equality up the prototype chain (#6151) #6153

lionel-rowe · 2024-10-28T13:33:15Z

Fixes #6151.
Fixes #6165.

Benchmarks below.

Benchmark summary:

Most cases are near-identical perf.
Temporal.PlainDate, Intl.Locale, URL, and URLPattern are all significantly slower (but within an order of magnitude), which I'd say is an acceptable tradeoff for now consistently giving correct results.
URLSearchParams is significantly faster (also within an order of magnitude) in addition to the consistently correct results.
Uint8Array is much faster (I only added a fast path for typed arrays to fix tests with subarray failing due to now walking the prototype chain).

// @deno-types="jsr:@std/assert@1.0.6/equal"
import { equal as current } from "https://jsr.io/@std/assert/1.0.6/equal.ts";
import { equal as next } from "./equal.ts";

const configs = [
  {
    group: "Temporal.PlainDate",
    one: () => Temporal.PlainDate.from("2023-01-01"),
    two: () => Temporal.PlainDate.from("2023-01-02"),
  },
  {
    group: "URLSearchParams",
    one: () => new URLSearchParams("date=2023-01-01"),
    two: () => new URLSearchParams("date=2023-01-02"),
  },
  {
    group: "URL",
    one: () => new URL("https://2023-01-01.com"),
    two: () => new URL("https://2023-01-02.com"),
  },
  {
    group: "URLPattern",
    one: () => new URLPattern("*://2023-01-01.com"),
    two: () => new URLPattern("*://2023-01-02.com"),
  },
  {
    group: "Intl.Locale",
    one: () => new Intl.Locale("es-MX"),
    two: () => new Intl.Locale("pt-BR"),
  },
  {
    group: "string",
    one: () => "2023-01-01",
    two: () => "2023-01-02",
  },
  {
    group: "Date",
    one: () => new Date("2023-01-01"),
    two: () => new Date("2023-01-02"),
  },
  {
    group: "number",
    one: () => 1,
    two: () => 2,
  },
  {
    group: "Array",
    one: () => ["2023-01-01"],
    two: () => ["2023-01-02"],
  },
  {
    group: "Set",
    one: () => new Set(["2023-01-01"]),
    two: () => new Set(["2023-01-02"]),
  },
  {
    group: "Object",
    one: () => ({ prop: "2023-01-01" }),
    two: () => ({ prop: "2023-01-02" }),
  },
  {
    group: "Map",
    one: () => new Map([["2023-01-01", 1]]),
    two: () => new Map([["2023-01-02", 1]]),
  },
  {
    group: "Uint8Array",
    one: () => new Uint8Array([1, 2, 3]),
    two: () => new Uint8Array([1, 4, 5]),
  },
];

for (const { group, ...x } of configs) {
  const one = x.one();
  const oneAgain = x.one();
  const two = x.two();

  for (const [name, equal] of Object.entries({ current, next })) {
    Deno.bench({
      group,
      name,
      fn() {
        equal(one, one);
        equal(one, oneAgain);
        equal(one, two);
      },
    });
  }
}

benchmark   time/iter (avg)        iter/s      (min … max)           p75      p99     p995
----------- ----------------------------- --------------------- --------------------------

group Temporal.PlainDate
current              1.0 µs       986,800 (626.1 ns …   1.6 µs)   1.2 µs   1.6 µs   1.6 µs
next                 3.8 µs       261,500 (  3.3 µs …   4.5 µs)   4.0 µs   4.5 µs   4.5 µs

summary
  current
     3.77x faster than next

group URLSearchParams
current             11.8 µs        84,960 (  7.3 µs …   1.3 ms)   9.1 µs  75.2 µs 103.9 µs
next                 3.1 µs       326,500 (  2.4 µs …   5.2 µs)   3.3 µs   5.2 µs   5.2 µs

summary
  next
     3.84x faster than current

group URL
current            508.5 ns     1,967,000 (395.4 ns … 811.8 ns) 554.4 ns 767.1 ns 811.8 ns
next               734.4 ns     1,362,000 (547.9 ns …   1.2 µs) 826.2 ns   1.2 µs   1.2 µs

summary
  current
     1.44x faster than next

group URLPattern
current            128.7 µs         7,771 ( 88.6 µs …   1.3 ms) 133.4 µs 467.0 µs 608.0 µs
next               328.8 µs         3,041 (158.5 µs …   6.1 ms) 395.7 µs   1.1 ms   1.2 ms

summary
  current
     2.56x faster than next

group Intl.Locale
current              1.2 µs       842,200 (714.0 ns …   1.8 µs)   1.4 µs   1.8 µs   1.8 µs
next                 4.9 µs       205,200 (  3.6 µs …   7.0 µs)   5.9 µs   7.0 µs   7.0 µs

summary
  current
     4.11x faster than next

group string
current            230.2 ns     4,344,000 (145.4 ns …   1.0 µs) 254.5 ns 478.3 ns 522.7 ns
next               198.7 ns     5,032,000 (131.5 ns … 488.2 ns) 234.2 ns 440.6 ns 440.8 ns

summary
  next
     1.16x faster than current

group Date
current            240.1 ns     4,165,000 (149.6 ns …   1.3 µs) 275.2 ns 553.2 ns 838.4 ns
next               202.8 ns     4,930,000 (137.0 ns … 546.8 ns) 228.6 ns 422.2 ns 497.3 ns

summary
  next
     1.18x faster than current

group number
current            224.9 ns     4,447,000 (132.0 ns … 583.8 ns) 261.9 ns 474.4 ns 483.4 ns
next               214.5 ns     4,662,000 (128.1 ns … 823.1 ns) 256.7 ns 468.1 ns 638.8 ns

summary
  next
     1.05x faster than current

group Array
current              4.8 µs       209,100 (  3.7 µs …   6.9 µs)   4.9 µs   6.9 µs   6.9 µs
next                 4.6 µs       219,500 (  3.8 µs …   6.0 µs)   4.8 µs   6.0 µs   6.0 µs

summary
  next
     1.05x faster than current

group Set
current            906.9 ns     1,103,000 (681.8 ns …   1.5 µs)   1.0 µs   1.5 µs   1.5 µs
next               920.7 ns     1,086,000 (652.2 ns …   1.7 µs)   1.0 µs   1.7 µs   1.7 µs

summary
  current
     1.01x faster than next

group Object
current              1.1 µs       887,000 (807.7 ns …   2.0 µs)   1.2 µs   2.0 µs   2.0 µs
next                 1.3 µs       754,400 (973.8 ns …   2.0 µs)   1.4 µs   2.0 µs   2.0 µs

summary
  current
     1.18x faster than next

group Map
current            678.0 ns     1,475,000 (444.3 ns …   1.3 µs) 794.7 ns   1.3 µs   1.3 µs
next               652.6 ns     1,532,000 (425.7 ns …   1.5 µs) 747.5 ns   1.5 µs   1.5 µs

summary
  next
     1.04x faster than current

group Uint8Array
current              6.2 µs       161,500 (  5.4 µs …   6.6 µs)   6.4 µs   6.6 µs   6.6 µs
next               327.1 ns     3,058,000 (248.6 ns … 608.9 ns) 354.3 ns 558.4 ns 608.9 ns

summary
  next
    18.93x faster than current

…#6151)

lionel-rowe · 2024-10-29T02:21:58Z

assert/equal.ts

+
+  function compare(a: unknown, b: unknown): boolean {
+    if (sameValueZero(a, b)) return true;
+    if (isPrimitive(a) || isPrimitive(b)) return false;


Front-loaded cheaper checks in an effort to make sure perf wasn't adversely affected for most cases (earlier versions seemed to have significant adverse impact on perf).

lionel-rowe · 2024-10-29T02:25:18Z

assert/equal.ts

-  return a.constructor === b.constructor ||
-    a.constructor === Object && !b.constructor ||
-    !a.constructor && b.constructor === Object;
+function prototypesEqual(a: object, b: object) {


Changed constructorsEqual to prototypesEqual because obj.constructor can easily be faked or inadvertently changed, whereas Object.getPrototypeOf can't (without monkey-patching Object.getPrototypeOf itself). This was necessitated due to some of the other changes affecting tests such as assertFalse(equal(new WeakMap(), { constructor: WeakMap }));

lionel-rowe · 2024-10-29T02:27:24Z

assert/equal.ts

+    pa === null && pb === Object.prototype;
+}
+
+function isBasicObjectOrArray(obj: object) {


When an object has null prototype or is a plain object/array, we don't care about checking other properties in its prototype chain.

lionel-rowe · 2024-10-29T02:27:49Z

assert/equal.ts

+    proto === Array.prototype;
+}
+
+// Slightly faster than Reflect.ownKeys in V8 as of 12.9.202.13-rusty (2024-10-28)


Not sure why this is the case 🤷

lionel-rowe · 2024-10-29T02:32:05Z

assert/equal.ts

+
+type TypedArray = Pick<Uint8Array | BigUint64Array, "length" | number>;
+const TypedArray = Object.getPrototypeOf(Uint8Array);
+function compareTypedArrays(a: TypedArray, b: TypedArray) {


Necessitated its own path due to equals() works with .subarray() test of bytes/equals_test.ts (otherwise the equality will fail due to checking byteOffset on the prototype).

lionel-rowe · 2024-10-29T02:32:53Z

assert/equal.ts

- * @param c The actual value
- * @param d The expected value
+ * @param a The actual value
+ * @param b The expected value


Hope it's OK to rename these, it seemed confusing that the variables a and b were named c and d in the public function.

lionel-rowe · 2024-10-29T02:34:11Z

assert/equal.ts

+      if (a instanceof TypedArray) {
+        return compareTypedArrays(a as TypedArray, b as TypedArray);
+      }
+      if (a instanceof WeakMap) {


Only necessary to check a for these checks now because prototypes are equal (rather than just any property that happens to be named "constructor").

codecov · 2024-10-29T04:06:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.53%. Comparing base (689fb69) to head (3500ebc).
Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6153   +/-   ##
=======================================
  Coverage   96.53%   96.53%           
=======================================
  Files         538      538           
  Lines       40902    40942   +40     
  Branches     6150     6150           
=======================================
+ Hits        39483    39522   +39     
- Misses       1375     1376    +1     
  Partials       44       44

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kt3k · 2024-11-06T07:27:03Z

Temporal.PlainDate, Intl.Locale, URL, and URLPattern are all significantly slower (but within an order of magnitude), which I'd say is an acceptable tradeoff for now consistently giving correct results.

This sounds fine to me as correctness matters more here.

URLSearchParams is significantly faster (also within an order of magnitude) in addition to the consistently correct results.
Uint8Array is much faster (I only added a fast path for typed arrays to fix tests with subarray failing due to now walking the prototype chain).

These are impressive improvements! Thanks for these works!

kt3k

LGTM

fix(assert): check property equality up the prototype chain (denoland…

e619624

…#6151)

lionel-rowe requested a review from kt3k as a code owner October 28, 2024 13:33

github-actions bot added the assert label Oct 28, 2024

lionel-rowe marked this pull request as draft October 28, 2024 13:51

lionel-rowe added 3 commits October 29, 2024 09:51

Fix failing Uint8Array tests

a3b5bb4

Fix type error in browser compat check

fd507b8

Merge branch 'main' into assert-equal-prototype

31acaf3

lionel-rowe commented Oct 29, 2024

View reviewed changes

lionel-rowe marked this pull request as ready for review October 29, 2024 02:35

lionel-rowe and others added 6 commits October 29, 2024 12:49

Consistently compare with prototype rather than constructor prop

d549ac6

Add test for unequal-length Uint8Array

2a4ee96

Merge branch 'main' into assert-equal-prototype

e07d572

Use sameValueZero for TypedArray entries

6383d20

Merge branch 'main' into assert-equal-prototype

a03866d

Add tests for constructor and prototype

3500ebc

kt3k mentioned this pull request Nov 6, 2024

assertEquals fails to throw on unequal instances of built-ins such as Temporal APIs (with --unstable-temporal flag) or URLSearchParams #6151

Closed

kt3k approved these changes Nov 6, 2024

View reviewed changes

kt3k merged commit 63fdf8d into denoland:main Nov 6, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(assert): check property equality up the prototype chain (#6151) #6153

fix(assert): check property equality up the prototype chain (#6151) #6153

lionel-rowe commented Oct 28, 2024 •

edited

Loading

lionel-rowe Oct 29, 2024

lionel-rowe Oct 29, 2024

lionel-rowe Oct 29, 2024

lionel-rowe Oct 29, 2024

lionel-rowe Oct 29, 2024

lionel-rowe Oct 29, 2024

lionel-rowe Oct 29, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

kt3k commented Nov 6, 2024

kt3k left a comment

fix(assert): check property equality up the prototype chain (#6151) #6153

fix(assert): check property equality up the prototype chain (#6151) #6153

Conversation

lionel-rowe commented Oct 28, 2024 • edited Loading

lionel-rowe Oct 29, 2024

Choose a reason for hiding this comment

lionel-rowe Oct 29, 2024

Choose a reason for hiding this comment

lionel-rowe Oct 29, 2024

Choose a reason for hiding this comment

lionel-rowe Oct 29, 2024

Choose a reason for hiding this comment

lionel-rowe Oct 29, 2024

Choose a reason for hiding this comment

lionel-rowe Oct 29, 2024

Choose a reason for hiding this comment

lionel-rowe Oct 29, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

kt3k commented Nov 6, 2024

kt3k left a comment

Choose a reason for hiding this comment

lionel-rowe commented Oct 28, 2024 •

edited

Loading

codecov bot commented Oct 29, 2024 •

edited

Loading