Add a per-pass profiler for the Arrow row diff - #98
Open
ksco92 wants to merge 4 commits into
Open
Conversation
The row diff re-reads each spooled input once per pass (hash, materialize, and the cell pass's two spills), and the serial re-read layer is the largest remaining term in the Arrow row diff's wall time. Measuring where the time goes needs a committed, reproducible harness rather than scratch instrumentation. Add a `profile` feature to onix-arrow that compiles per-pass wall-time and peak-RSS boundaries into diff_rows and the streaming cell pass. The boundaries expand to nothing without the feature, which is off by default and never enabled by the release wheel, so the instrumentation is absent from the shipped library (verified with strings). Peak RSS is sampled from ps in a background thread, so the feature adds no dependency and no unsafe. The row_diff_profile example (built only with the feature) spools two generated sides to anonymous Arrow IPC files and prints the per-pass table, reproducing the re-read layer the Python bindings' input spool incurs. Extract materialize_row_members so the added pass boundaries keep diff_rows within the line budget; behaviour is unchanged.
Add a Profiling section to the perf README naming the row_diff_profile example, its exact command, and the rule that every row-diff performance change posts its before/after per-pass table from it; the example's module docstring is the single home for the method. Add a Per-pass profile section to RESULTS.md with the narrow- and wide-shape tables at 1M rows and the re-read breakdown the numbers show.
- Factor the shared streaming generator (Shape, Generated, GenReader, its TableInput/RecordBatchReader impls) into examples/shared/gen_shapes.rs, included by both row_diff_rss and row_diff_profile; row_diff_rss's generated data is unchanged (same code, same counts). - profile: keep the sampler JoinHandle in state and join it in finish(), so a back-to-back begin()/finish() can never leave a second sampler running; add a test over 20 cycles asserting no sampler outlives finish(). Sample RSS through the absolute /bin/ps rather than resolving ps via PATH, and say the sampler is ps-based (macOS/Linux) in the module doc. - perf docs: state the per-pass table is deepdiff-rs 0.11.2, medians of 11 runs, and update the environment row; drop the repeated spool-rationale restatements and the follow-up design mechanism, keeping the removable-re-read observation with a pointer to #90.
- Instrument the spool decode (spool reader next()) and the spill routing (take + cast) so the per-pass decomposition closes and shows decode, not the key re-hash, as the dominant re-read term. - Gate recording on a begin()/finish() window so a diff built with the feature but not being profiled records nothing (and cannot grow the global state), and the profiler's own tests are deterministic under concurrent diffs. - Give the example a file mode that reads each side from an uncompressed Arrow IPC file with --key/--threads (the real fixtures, converted once from parquet with pyarrow), alongside the generated proxy mode; time an uninstrumented run next to the instrumented one so the reported wall excludes the ps sampling cost. - Add a mutant-catching test: a right side whose key and value columns sit at different positions than the left pins that each side materializes through its own SideColumns. - profile.rs tests (finish joins the sampler, a second begin spawns none, label/accum ordering, ps of a missing pid is None); add make test-all-features to check so the feature-on instrumentation and these tests run in the gate. - perf docs: real narrow/wide fixtures at 1M and full in the RESULTS per-pass section (decode-dominated, decomposition closing), proxies labelled as proxies; document the file mode and the pyarrow conversion in the README.
ksco92
force-pushed
the
perf/90-spool-reread
branch
from
September 6, 2026 17:23
86bbb7f to
4257241
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
profilefeature toonix-arrowthat compiles per-pass wall-time and peak-RSS boundaries intodiff_rowsand the streaming cell pass. The boundaries expand to nothing without the feature (off by default, never enabled by the release wheel), so the instrumentation is absent from the shipped library — verified withstringsonlibdeepdiff_rs.dylib(0 matches for the pass labels and the sampler thread name; present in the feature-built example). Recording is gated on abegin/finishwindow; RSS is sampled through/bin/psin a background thread thatfinishjoins. No new dependency, nounsafe.row_diff_profileexample (built only with the feature) with two modes: file (reads each side from an uncompressed Arrow IPC file,--key/--threads) for the real fixtures, and generated (deterministic proxy shapes) for a dependency-free check. It prints an uninstrumented wall, an instrumented wall, and the per-pass table (wall + peak RSS).spool decode (reader.next)) and the spill routing (cell: spill route) so the per-pass decomposition closes (top-level passes sum to within 1% of the total) and shows decode, not re-hashing, as the dominant re-read term.row_diff_rssandrow_diff_profile(examples/shared/gen_shapes.rs);row_diff_rss's generated data and counts are unchanged.deepdiff-rs0.11.2.make test-all-features(cargo test --workspace --all-features) tocheckso the feature-on instrumentation and the profiler's own tests run in the gate.Issues
Baseline per-pass profile (real fixtures, file mode, 18 threads)
Medians (11 at 1M, 5 at full), no contending process. Each cell is
wall s (peak RSS MB).Peak RSS (median of per-pass peaks): narrow full ~4.0 GB, wide full ~19.7 GB. Full RESULTS.md has the RSS-per-pass and the route/write/read-back/render sub-rows.
Where the time goes (decode dominates the re-read)
The re-read is decode-bound. The
decode (reader.next)accum is 60–66% of the combined hash + materialize + spill-re-read wall at every size, andmaterialize(re-reads with an empty select) is essentially all decode — scanning every key value adds only a few ms. So the re-read optimization wins by decoding each side once instead of three times, not by avoiding the key re-hash. Onnarrow fullthe three re-read rounds are 91% of the total (4.45 of 4.87 s); onwide fullthey are 32% (the rest is the cell pass's own route/render/reorder).Stop-and-report triage (issue #90's decode clause)
#90 says to stop if the parquet→IPC decode itself dominates. It does dominate the re-read rounds — but the design still wins, because it removes two of the three decodes: the materialize and cell passes read only their key-hash partition, written once during the first read, so the input is decoded once, not three times. Deliverable 2 proceeds on that corrected premise. Expected saving from the corrected split: removing the materialize re-read and the cell-spill re-read (both decode-dominated) is the bulk of the re-read term — 91% of
narrow full, and the 32% re-read share ofwide full.Test Results
cargo fmt --check;cargo clippy --all-targets --all-features -- -D warnings)cargo test --workspaceandcargo test --workspace --all-features)cargo llvm-cov --workspace --fail-under-lines 95: 97.19% total,row_diff.rs97.61%).profile.rscompiles only under the feature, so it is outside the default-feature coverage build; measured under--all-featuresit is 99.44% lines (and the--all-featurestotal is 97.21%).cargo mutants --in-diffover the changedrow_diff.rslines (default build): 11 tested, 6 caught, 5 unviable, 0 missed — including thecolumns: ctx.right_columnsmutant inmaterialize_row_members, now caught by the different-column-order test. (profile.rsis feature-gated and not in the default mutants build; it is exercised by its own tests under--all-features, 99.44% line coverage.)cargo doc -D warnings,cargo deny check,cargo machete)diff_tablesreturns correct counts through pyarrow on 3.14.Notes
pscost (the timer starts afterenter's RSS read, stops before the guard's), so they sum to about the uninstrumented wall; the instrumented wall is reported separately.