Skip to content

test: make highway-strings.test.ts assert per sweep instead of per case - #37488

Open
robobun wants to merge 1 commit into
mainfrom
farm/868897b1/fast-highway-strings-test
Open

robobun wants to merge 1 commit into
mainfrom
farm/868897b1/fast-highway-strings-test

Conversation

@robobun

@robobun robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator

Test-only change to test/js/bun/util/highway-strings.test.ts, flagged by the slow-test sweep (15.2s on the debian 13 x64-asan lane in build #91993, ~8s on release lanes).

Why it was slow

The file made 46,341 expect() calls, one per (kernel, length, offset, position). The CI runner sets BUN_GARBAGE_COLLECTOR_LEVEL=1, and under that setting every matcher call also requests a GC (Expect::post_match -> auto_garbage_collect), so the expects were the cost, not the kernels: a kernel call through the test binding is a few microseconds even on a debug build. On top of that, the indexOfAny case rebuilt its haystack and ran a Uint8Array#includes-per-byte reference for every planted position; on a debug build that one case alone takes ~9.5s, so bun bd test of this file on main currently fails with the default 5s per-test timeout (it only passes in CI because the runner raises the timeout).

What changed

  • Every case's answer is known by construction (filler bytes are all >= 0x80, needles are all < 0x80, and we know where we planted them), so the scalar reference functions are gone. The two-needle / second-member / second-copy cases are min/max/pos/later; the dense countChar counts are ceil(len / 3).
  • Each (length, offset) haystack is built once per test; the needle is planted and restored in place instead of regenerating the buffer per position.
  • check() calls the kernel and records a wrong result as a string; an afterEach asserts the list is empty (first 25 shown plus the total), so every test asserts exactly once and a failure names every bad case, e.g. lastIndexOfAny set=3 len=1029 off=13 pos=1013: got 1029, want 1013. 15 expect() calls total; 67,302 kernel results are checked (the old file checked 46,341).
  • positions() is memoized; recomputing it on every call was 15-20% of the remaining debug-build time.

Nothing was dropped: same LENGTHS, same OFFSETS, same needle and set lists, and every position the old positions() produced is still produced.

Coverage added

  • positions() also yields both sides of every 16-byte boundary counted from the end of the haystack. LastIndexOfCharImpl, LastIndexOfAnyCharImpl and MemMemReverse step back from the end, so for lengths that are not a multiple of the vector width (1029, 257, 129, ...) their lane boundaries are at len - k*N, which the old start-relative positions never hit (1029 goes from 197 to 389 positions).
  • countChar on 256 * 64 + 7 bytes: CountCharImpl flushes its per-lane u8 counters every 255 vectors, and 1029 bytes never reached a flush on any target.
  • memmem / memrmem where every start passes the two-anchor filter (needle aba in an all-a haystack): exercises the verify-and-clear-lane loop, and for lengths >= 127 the false-positive budget (2*len/needle + 32) trips into the Two-Way fallback, which then has to find a copy planted past the resume point (memmem) or before it (memrmem). The explicit fallback cases in buffer-indexof-worstcase.test.ts all use absent needles.
  • The decoy test also plants abxd decoys (pass the anchors, fail the memcmp) and a needle truncated at the very end of the view with its last byte sitting in the padding right after it.
  • Every haystack now has 64 bytes of padding on each side filled with the bytes being searched for, so a kernel that reads outside the view reports a hit (or count) the view does not contain. The Buffer test wraps the same view zero-copy, so it also runs with a non-zero byteOffset.
  • A final test asserts Object.keys(driven) equals the explicit sorted kernel list, and that the binding rejects an unknown op. The binding dispatches on an op string, so there is no runtime key list to diff against; this catches a kernel dropping out of the sweeps, not a new op added to the binding without coverage.

Verification

CI on this branch (build #92216), the file itself: 428ms on the debian 13 x64-asan lane (15.2s in build #91993, the run that flagged it) and 66ms on debian 13 x64 (~8s before).

Debug ASAN build (bun bd) of da3851e, this file only:

main this PR
bun bd test (defaults) 18.56s, 1 fail (indexOfAny case times out at 5s) 4.51s, 11 pass
same with the runner's BUN_GARBAGE_COLLECTOR_LEVEL=1 91.4s 4.77s
expect() calls 46,341 15
kernel results checked 46,341 67,302

Of the 4.5s, ~2.4s is bun test startup plus test/preload.ts on a debug build (an empty test file in this directory takes the same), so the test bodies went from ~16s / ~89s to ~2.1s. On a release build with the runner's GC setting the file goes from 3.6-4.3s to 0.2s.

Fault injection (scratch copy of the file forcing lastIndexOfChar / lastIndexOfAny to miss position 1013 of a 1029-byte haystack and memrmem to miss position 240 of a 257-byte one) fails 5 of the 11 tests and lists each case by kernel, length, offset and position; position 1013 is one of the new end-relative boundaries.

The file made 46,341 expect() calls. Under the CI runner's
BUN_GARBAGE_COLLECTOR_LEVEL=1 every matcher call also requests a GC, so
the expects, not the kernels, were the cost (91s on a debug ASAN build,
15s on the release ASAN lane). The indexOfAny case also rebuilt its
haystack and ran a Uint8Array#includes reference per planted position,
which alone exceeded the default 5s per-test timeout under bun bd test.

Every case's answer is known by construction, so the scalar references
are gone, each (length, offset) haystack is built once and the needle is
planted and restored in place, and wrong results are collected as
strings and asserted empty once per test (15 expect() calls, 67,302
kernel results checked). Coverage added on top of the previous lengths,
offsets and positions: lane boundaries counted from the end of the
haystack for the reverse kernels, the 255-vector flush in countChar,
memmem/memrmem with every start passing the anchor filter (verify loop
and Two-Way fallback, both directions), a needle truncated at the end of
the view, needle bytes in the padding around every haystack, and a final
test asserting every kernel the binding dispatches was driven.
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 25 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: cb312a83-6fe9-45cc-828c-d67f3850eb7b

📥 Commits

Reviewing files that changed from the base of the PR and between da3851e and b96b079.

📒 Files selected for processing (1)
  • test/js/bun/util/highway-strings.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:12 AM PT - Aug 11th, 2026

✅ @robobun, your commit b96b079d0b205aedcf358dbf77356cace55af5e9 passed in Build #92216! 🎉


🧪   To try this PR locally:

bunx bun-pr 37488

That installs a local version of the PR into your bun-37488 executable, so you can run:

bun-37488 --bun

@robobun

robobun commented Aug 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review.

Test-only change (no src/ changes). CI build #92216 on this branch, the file itself: 428ms on the debian 13 x64-asan lane (15.2s in build #91993, the run that flagged it) and 66ms on debian 13 x64 (~8s before). Locally on a debug ASAN build of da3851e: bun bd test 18.56s with the indexOfAny case failing the default 5s per-test timeout on main, 4.51s and 11 pass here; with the runner's BUN_GARBAGE_COLLECTOR_LEVEL=1 91.4s on main, 4.77s here. expect() calls 46,341 -> 15, kernel results checked 46,341 -> 67,302. Details and the added coverage are in the PR description.

Self-review is running; anything it turns up that holds will be pushed to this branch.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — test-only refactor that speeds the file up ~20x while strictly expanding coverage.

What was reviewed:

  • Traced the save/restore-in-place pattern in every sweep (two-needle mirror pairs, h2.set(saved, pos) where saved came from h — valid because haystack() is deterministic on (len, off) and the second copy at later never overlaps pos).
  • Checked the computed-by-construction expected values match what the deleted scalar refs would have returned: min/max for two needles, ceil(len/3) for the dense count, pos/pos+2 for the indexOfAny second-member case, and the aba-in-all-a and decoy cases can't produce accidental matches.
  • Confirmed the afterEach mismatch check resets before asserting so a failing test doesn't poison the next, and that KERNELS is already alphabetically sorted so Object.keys(driven).sort() matches.
Extended reasoning...

Overview

Rewrites test/js/bun/util/highway-strings.test.ts (single file, test-only) to collapse ~46k expect() calls into one per test via a mismatch-collecting check()/afterEach pair, replaces the scalar reference functions with expected values known by construction (filler ≥ 0x80, needles < 0x80, planted at known positions), reuses one haystack per (len, off) with save/plant/restore instead of rebuilding per position, memoizes positions(), and adds coverage: end-relative lane boundaries, a countChar length that forces the 255-vector flush, an anchors-always-pass memmem/memrmem case that reaches the Two-Way fallback, abxd decoys and a truncated-needle-at-view-end case, poisoned padding on both sides of every view, and a final assertion that all 8 kernel ops were driven.

Security risks

None. No production code, no network, no filesystem writes outside the test's own typed arrays, no external input.

Level of scrutiny

Medium — it's a full rewrite of a test's assertion strategy, so the main risk is silently weakening coverage (a repo review rule). I traced each deleted ref* call to its new closed-form expected value and they agree; LENGTHS, OFFSETS, needle/set lists are unchanged and positions() is a strict superset of the old output. The save/restore pattern is correct in every case including the shared-saved reuse across h/h2 (both are deterministic on the same seed and the restored range never overlaps later). If any expected value were wrong CI would fail immediately, so residual risk is low.

Other factors

No CODEOWNER on this path. PR description includes debug-ASAN timings and fault-injection verification showing the new end-relative positions catch injected faults. The bug hunting pass found nothing. This PR is already merged as HEAD (b96b079) in the checkout, so it has presumably passed CI.

@robobun

robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

This file is still slow on main: 98s on the alpine 3.23 aarch64 lane in build 110747 (the fifth slowest file in that run). This PR is the fix for that, but it now conflicts with main. #39616 (88165c6) added the memmem16 / memrmem16 sweeps to the same file after this branch was cut. Those two tests use the same per-case expect() pattern and need the same per-sweep treatment when this is rebased.

This branch has not been deployed

No deployments
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.

1 participant