perf(buf): route findIterBuf through packed transport - #71
Conversation
findIterBuf went via the per-match napi Match path, costing ~6-11x vs findIter on large inputs. Route it through _findIterPackedBuf and unpack a Uint32Array on the JS side. Byte offsets are preserved (no UTF-16 translation on the buffer path).
Self-contained benchmark that synthesises a haystack producing 44k matches, runs findIter and findIterBuf on the same input, and fails (exit 1) if the buffer path drifts to more than 2x the string path. Wired into bench:all and exposed as bench:buf. Also add three targeted findIterBuf tests covering ASCII byte/UTF-16 parity with findIter, large match counts (10k via packed Uint32Array), and the empty result case.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes findIterBuf by implementing a packed Uint32Array transport mechanism, aligning it with the performance characteristics of the string-based findIter. Key changes include the addition of a performance benchmark, new test cases for large match counts and ASCII consistency, and the implementation of the unpackBuf utility. Feedback was provided regarding the safety of array initialization in unpackBuf, suggesting the use of Math.floor to avoid potential RangeError exceptions if the underlying data transport is malformed.
| function unpackBuf(packed: Uint32Array): ByteMatch[] { | ||
| const len = packed.length; | ||
| // eslint-disable-next-line unicorn/no-new-array | ||
| const matches = new Array<ByteMatch>(len / 3); |
There was a problem hiding this comment.
The new Array(len / 3) call will throw a RangeError: Invalid array length if len is not a multiple of 3 (since it will be a floating-point number). While the native side is expected to return triples, it's safer to use Math.floor() to ensure a descriptive error is thrown by the subsequent logic instead of a cryptic RangeError if the transport is ever malformed.
| const matches = new Array<ByteMatch>(len / 3); | |
| const matches = new Array<ByteMatch>(Math.floor(len / 3)); |
There was a problem hiding this comment.
Agreed — new Array(len / 3) with a non-integer would throw RangeError: Invalid array length before the per-triple guard could surface a descriptive message. Fixed in ffc645c by wrapping the division in Math.floor. Applied the same change to the sibling unpack helper on line 144 since it carried the same risk.
CC on behalf of @jan-kubica
`new Array(packed.length / 3)` would throw a cryptic `RangeError` if the native side ever returned a length that is not a multiple of 3. Wrap the division in `Math.floor` so the descriptive per-triple guard fires instead. Applied to both `unpack` and `unpackBuf` to keep the unpack family consistent. Addresses gemini-code-assist review on #71.
|
Rabbit round on the gemini-code-assist review. Summary review: the only actionable point was the Quality checks re-run locally: CC on behalf of @jan-kubica |
1 participant
findIterBuf wasn't routed through the packed Uint32Array transport that findIter uses, costing ~6-11x on large inputs (44k matches in our self-contained bench, audit reported 226ms vs 21ms on 140k matches). The Rust
_findIterPackedBufalready existed atsrc/lib.rs:694; the JS wrapper insrc/core.tswas still calling the per-matchfindIterBufnative path that round-trips aVec<Match>across FFI. Plumbed the JS side through_findIterPackedBufplus aByteMatchunpacker. Added a bench assertion to keep findIterBuf within 2x of findIter, plus three targeted tests (ASCII parity with findIter, 10k-match packed-transport stress, empty result).Offsets stay in bytes on the buffer path (UTF-16 translation only happens for the string API in Rust);
ByteMatchkeeps itspattern/start/end-only shape and the existing "no text field" test still passes.Test plan