Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(bytes): equals() works with subarray #4630

Merged
merged 2 commits into from
Apr 23, 2024
Merged

fix(bytes): equals() works with subarray #4630

merged 2 commits into from
Apr 23, 2024

Conversation

iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Apr 23, 2024

Fixes #3603

@iuioiua iuioiua requested a review from kt3k as a code owner April 23, 2024 07:17
@github-actions github-actions bot added the bytes label Apr 23, 2024
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.99%. Comparing base (d209aa3) to head (0052a36).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4630   +/-   ##
=======================================
  Coverage   90.99%   90.99%           
=======================================
  Files         478      478           
  Lines       37383    37383           
  Branches     5308     5308           
=======================================
  Hits        34016    34016           
  Misses       3305     3305           
  Partials       62       62           

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

@iuioiua iuioiua enabled auto-merge (squash) April 23, 2024 07:43
const compressedA = new Uint32Array(a.buffer, 0, compressible);
const compressedB = new Uint32Array(b.buffer, 0, compressible);
const compressedA = new Uint32Array(a, 0, compressible);
const compressedB = new Uint32Array(b, 0, compressible);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@iuioiua iuioiua merged commit 3149cdb into main Apr 23, 2024
15 checks passed
@iuioiua iuioiua deleted the fix-bytes-subarray branch April 23, 2024 07:50
@tjjfvi
Copy link
Contributor

tjjfvi commented Apr 23, 2024

This change fixes the bug but is really bad for performance:

import { equals } from "https://raw.githubusercontent.com/denoland/deno_std/0052a36/bytes/equals.ts"

const a999 = new Uint8Array(999);
const b999 = new Uint8Array(999);

const a1k = new Uint8Array(1000);
const b1k = new Uint8Array(1000);

Deno.bench("999", () => {
  equals(a999, b999)
})

Deno.bench("1k", () => {
  equals(a1k, b1k)
})
benchmark      time (avg)        iter/s             (min … max)       p75       p99      p995
--------------------------------------------------------------- -----------------------------
999           461.05 ns/iter   2,168,977.3 (454.72 ns … 511.36 ns) 460.35 ns 487.23 ns 511.36 ns
1k              1.37 µs/iter     728,273.7     (1.29 µs … 1.53 µs)   1.41 µs   1.53 µs   1.53 µs

The "optimized" equals32Bit function is 3x slower than the naive version.

As @0f-0b pointed out, the new Uint32Array(a, ...) is copying each element; new Uint32Array(new Uint8Array([1, 2])) is new Uint32Array([1, 2]). So, it's currently, taking two N-byte buffers, allocating two 4N-byte buffers, copying the N bytes to every fourth byte, and then checking if all 4N bytes are equivalent (75% of which are zeroes).

I don't think this function should be allocating memory at all (for one thing, it makes the function always O(N), even if the first byte differs). Instead, the equals32Bit path should be taken only when a.byteOffset % 4 === b.byteOffset % 4, and the uint32arrays should be constructed as subarrays.

@iuioiua
Copy link
Contributor Author

iuioiua commented Apr 23, 2024

Thank you for the insight, @0f-0b and @tjjfvi. You guys are right. I'll submit a PR that reverts this change shortly.

@iuioiua
Copy link
Contributor Author

iuioiua commented Apr 23, 2024

See #4632

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bytes/equals incorrectly handles subarrays
4 participants