Skip to content

perf(rust): reduce sort32 bucket overhead - #398

Open
dexhunter wants to merge 1 commit into
sparkjsdev:mainfrom
dexhunter:perf/sort32-review-step1-20260715
Open

perf(rust): reduce sort32 bucket overhead#398
dexhunter wants to merge 1 commit into
sparkjsdev:mainfrom
dexhunter:perf/sort32-review-step1-20260715

Conversation

@dexhunter

@dexhunter dexhunter commented Jul 15, 2026

Copy link
Copy Markdown

Summary

  • count valid keys in fixed-size chunks to reduce loop overhead
  • reuse one mutable bucket reference per scatter instead of indexing each bucket twice
  • return before the scatter passes when there are no active splats
  • add regression tests for invalid keys, finite-value boundaries, and stable ordering

Performance status

The earlier 24.0% latency-reduction headline is withdrawn pending revalidation.

A fresh audit found that the retained evaluator used Node's native WebAssembly runtime but did not record the Node/V8, Rust, wasm-bindgen, or host versions. Its runner contains seven mixed cases, not the ten previously stated, and every case injects invalid keys. It has no all-valid radial-depth fixture or per-layout paired baseline/candidate receipts, so it does not establish an improvement for the common all-valid case.

The required follow-up is a pinned-toolchain, alternating paired rerun with valid-only production-like radial-depth inputs reported separately from mixed-invalid controls, including raw samples and per-layout medians.

Historical autoresearch trace (not validation evidence): https://dashboard.weco.ai/share/4avZ2RktRLkZmJ1sWBU1Tiqqd20Zndg7

Validation

  • cargo test --manifest-path rust/spark-rs/Cargo.toml (2 passed)
  • evaluator evidence audit completed; corrected valid-only replay pending
  • git diff --check

Co-Authored-By: Aiden <aiden@weco.ai>
@dexhunter

Copy link
Copy Markdown
Author

Rechecked against current main (fec6d05): this still merges cleanly, and rust/spark-rs/src/sort.rs has not changed upstream since #327, so the patch is still confined to that one file (+53/-16).

The amber check state is not a failure — Spark CI Linux and Spark CI Windows are both sitting in action_required awaiting fork-workflow approval, so they have never actually run. I ran the same steps locally against main plus this patch: npm run lint clean across 84 files, npm run test 1/1, and cargo test --manifest-path rust/spark-rs/Cargo.toml 2/2 (the two regression tests this PR adds).

Separately, and this applies to any Rust-only change rather than to this PR specifically: build-dist.yml triggers on paths: ['src/**'], so a commit confined to rust/ does not regenerate the committed dist/ bundles that inline the wasm. #327 landed on 2026-06-08 and the next Bump dist build was 2026-07-15 (8a033bc1), immediately after the unrelated src/ change in #378 — so that sort work reached dist/ about five weeks after it merged. Happy to add rust/** to that trigger, here or as a separate PR, if that would be useful — or to leave it alone if the dist refresh is meant to be deliberate.

@mrxz

mrxz commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Apologies for the late response, looking into the linked "Autoresearch trajectory" I'm not so sure about the stated 24.0% latency reduction. Would it be possible to know the actual test data/suite and which runtime/engine was used to execute these test, as WASM performance can differ quite a bit between browsers/runtimes.

The largest gain seems to come from the conditional when tallying. In practice there really shouldn't be many (if any) invalid depth values, as the default sort metric is based on radial depth (always positive). Seeing it have such an impact is somewhat unexpected. If it doesn't have any negative performance impact on the common case, it might still be worth considering, of course.

Separately, and this applies to any Rust-only change rather than to this PR specifically: build-dist.yml triggers on paths: ['src/**'], so a commit confined to rust/ does not regenerate the committed dist/ bundles that inline the wasm.

Good point, created a PR #412

@dexhunter

Copy link
Copy Markdown
Author

This is Aiden on behalf of Dex

You're right to question the 24.0% figure. I audited the retained evaluator instead of inferring from its aggregate result. It used Node's native WebAssembly runtime, but the artifacts do not record the Node/V8, Rust, wasm-bindgen, or host versions. More importantly, the retained runner contains seven cases—not the ten stated in the PR—and every case injects invalid keys at fixed intervals. It has no all-valid radial-depth fixture or per-layout paired baseline/candidate receipts, so the current evidence does not support that headline for the common all-valid case.

I'm treating the 24.0% claim as unverified. The required follow-up is a pinned-toolchain, alternating paired rerun with valid-only production-like radial-depth inputs, reported separately from mixed-invalid controls, with raw samples and per-layout medians. I'll update the performance claim from that evidence before asking you to evaluate the optimization.

@dexhunter
dexhunter marked this pull request as draft August 26, 2026 13:57
@dexhunter
dexhunter marked this pull request as ready for review August 26, 2026 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants