Skip to content

Benchmark public PCO selections and cold/retained probe batches - #10824

Draft
danking wants to merge 10 commits into
developfrom
dk/pco-selection-benchmarks
Draft

danking wants to merge 10 commits into
developfrom
dk/pco-selection-benchmarks

Conversation

@danking

@danking danking commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Add 26 public PCO Divan cases covering write/full decode, prepared-buffer metadata open/drop batches, contiguous/scattered/repeated Take, near-full Slice, compact ranges, and scalar probes. Each workload checks dtype, logical nulls, values and ordering against a public primitive reference outside the timed closure.

Work counts are fixed across every implementation: 4,194,304 source rows; 4,194,304 outputs for the narrow-view repeat control; 256 fresh-state probes or 1,048,576 retained probes. Fresh probes use a prepared owner, with index 8191 defined and 8190 missing; metadata batches contain 256 opens plus immediate destruction and exclude IO. The final narrow calibration measured 21.17 ms dense and 3.247 ms nullable on the cloud OS timer: the nullable control remains below the tens-of-milliseconds target, and the locked count is not adjusted per implementation. Historical undersized and overshot calibration logs are retained.

First of six replacements for closed #10771. The final new-only matched 26-case resource run completed with all untimed oracles passing. Current GitHub CI and human review remain separate gates.

Existing CI and CodSpeed are reused; no new workflow. AI-assisted development with OpenAI Codex. Normal CI and the required human reviews remain necessary before merge; this PR remains draft.

Final CI audit: source/platform/formatting/TSAN checks have passed; remaining performance gates are CodSpeed and required human review. All PRs remain draft; no performance waiver or merge approval is claimed.

Signed-off-by: Daniel King <dan@spiraldb.com>
… timing

Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
@codspeed

codspeed Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Merging this PR will improve performance by 12.6%

4 warnings, 1 note

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚠️ Unable to measure the execution of 2 benchmarks

These benchmarks measured no execution time, so their results are not comparable and count as unchanged. This usually happens when the compiler removes the code under test.

Prevent compiler optimizations

See affected benchmarks

⚠️ 16 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ Base and head ran on different runner types

Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.

Learn more about Macro Runners

Open the report in CodSpeed to investigate

ℹ️ 40 benchmarks have no flamegraph

The flamegraph wasn't generated: the symbols of the profiling trace could not be resolved, because no information about the language frames was collected.

See affected benchmarks

⚡ 4 improved benchmarks
✅ 2435 untouched benchmarks
🆕 26 new benchmarks
⏩ 12 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ CPU Simulation test_scan_filter_to_arrow_array[int_float] 4 ms 3.5 ms +15.94%
⚡ CPU Simulation test_repeated_scan_filter[int_float] 3.8 ms 3.4 ms +12.47%
⚡ CPU Simulation test_repeated_scan_filter[int] 2.7 ms 2.4 ms +11.2%
⚡ CPU Simulation test_scan_filter_to_arrow_array[int] 2.8 ms 2.5 ms +10.87%
🆕 CPU Simulation first_probes[false] N/A 121.8 ms N/A
🆕 CPU Simulation first_probes[true] N/A 121.7 ms N/A
🆕 CPU Simulation full[false] N/A 344.5 ms N/A
🆕 CPU Simulation full[true] N/A 540.1 ms N/A
🆕 CPU Simulation metadata_open_batch[false] N/A 384.6 ms N/A
🆕 CPU Simulation metadata_open_batch[true] N/A 346.7 ms N/A
🆕 CPU Simulation retained_probes[false] N/A 346.8 ms N/A
🆕 CPU Simulation retained_probes[true] N/A 463.1 ms N/A
🆕 CPU Simulation selection[("broad", false)] N/A 381.2 ms N/A
🆕 CPU Simulation selection[("broad", true)] N/A 577.9 ms N/A
🆕 CPU Simulation selection[("near_full", false)] N/A 345.6 ms N/A
🆕 CPU Simulation selection[("near_full", true)] N/A 540.8 ms N/A
🆕 CPU Simulation selection[("ranges", false)] N/A 345.3 ms N/A
🆕 CPU Simulation selection[("ranges", true)] N/A 540.4 ms N/A
🆕 CPU Simulation selection[("repeated", false)] N/A 353.5 ms N/A
🆕 CPU Simulation selection[("repeated", true)] N/A 549 ms N/A
... ... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing dk/pco-selection-benchmarks (2736d4c) with develop (a59294e)

Open in CodSpeed

Footnotes

  1. 12 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@danking danking added the changelog/chore A trivial change label Oct 26, 2026
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
@danking

danking commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @danking's task in 59s —— View job


Review: PCO selection benchmarks

This adds benchmarks only, with no production code. The oracle checks before each timed loop are good: every workload is checked for correctness against the uncompressed PrimitiveArray before it is timed. I found no correctness bugs. Most of the feedback is about how much CI time these cost and how readable the reports are.

Should fix before un-drafting

  1. The workloads are about 10× over the stated budget. The PR body aims for "tens of milliseconds" per operation. CodSpeed reports much more:

    Benchmark CodSpeed time per op
    full[true] 540 ms
    selection[("broad", true)] 577 ms
    retained_probes[true] 463 ms
    metadata_open_batch[false] 385 ms
    first_probes[*] 122 ms

    That is for 40 new cases in the shared encoding shard, and the CPU-simulation shard runs under valgrind, so wall-clock time is much higher still. Consider lowering ROWS from 4M to 1M and PROBES from 1M to about 64K. Per-probe numbers come from dividing by the count anyway, so the reporting stays the same.

  2. Fixtures are rebuilt and re-checked for every case (compressed() / input(), selection.rs:45-63).

    • Every case of every bench function compresses 4M rows from scratch.
    • Several cases also build input() two or three times: in compressed, in reference, and in the oracle.
    • None of this is timed, but under CodSpeed simulation it dominates the job's wall time.
    • Fix: cache them once per nullability, e.g. static FIXTURES: LazyLock<[(PrimitiveArray, ArrayRef); 2]>. Then call LazyLock::force in main like scalar.rs does.

    Fix this →

  3. CodSpeed says one benchmark measured no execution time. It counts as unchanged, so it currently guards nothing. Find out which case it is in the "optimized_out" filter on CodSpeed. Then either make its result observable (black_box the input as well as the output) or drop the case.

Smaller suggestions

  1. The _ => arm hides mistakes (selection.rs:149). It silently uses [0] as the indices. For near_full that value is never used, but a typo in SELECTIONS would turn into a 1-row take that still passes its oracle check.

    • Better: handle near_full explicitly (e.g. Option<ArrayRef>) and make the fallback unreachable!("unknown selection {kind}").
    • Also, selection takes a slice path for near_full and wraps a view for narrow_repeated, picked by kind == string comparisons in three places. An enum Selection with Display would be clearer and would make bad cases impossible to write.
  2. The bool args are hard to read in reports.

    • full[true], first_probes[false] and metadata_open_batch[true] don't say whether true means nullable or missing.
    • In first_probes the bool means missing; everywhere else it means nullable. The two are easy to mix up on CodSpeed.
    • Named string args, or an enum with Display ("nullable" / "non_nullable", "defined" / "missing"), would make the dashboard readable.
  3. Missing global allocator. scalar.rs and the other encoding benches install #[global_allocator] static GLOBAL: MiMalloc, and mimalloc is already a dev-dependency. Without it these numbers use the system allocator. That matters most for metadata_open_batch and write, which allocate on every call.

  4. retained_probes overlaps scalar.rs.

    • Every one of its probes falls in 8192..8448. With values_per_page = 1024 that is a single page, so this is the same "clustered within one page" case scalar.rs already measures, just with 1000× more iterations.
    • If the intent is to measure keeping decoded state across pages, add a case that crosses pages.
    • If not, update the module doc to say this is single-page.
  5. metadata_open_batch seems to decode everything. It reports about the same time as full[false] (385 ms vs 344 ms). That suggests deserialization currently decodes or validates all pages, which is probably the very thing a later PR in the series will improve. Worth confirming that is expected, because the comment at selection.rs:268 says this "measures metadata/index preparation".

  6. Nit: the PR body is missing some spaces: "are256", "are1,048,576", "below10ms".

Unrelated CodSpeed regressions

The six regressions in the CodSpeed comment are in vortex-array, runend and the Python filter benches. This PR does not touch them, and CodSpeed warns that base and head ran on different runner types. They look like noise.

Checks

I only read the code. I did not run clippy, fmt or the benches; your CLAUDE.md leaves checks to you, and the PR already notes that scoped compile and lint are pending.

Signed-off-by: Daniel King <dan@spiraldb.com>
…load

Signed-off-by: Daniel King <dan@spiraldb.com>
@danking

danking commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @danking's task in 51s —— View job


Review: PCO selection benchmarks

  • Gather context
  • Read encodings/pco/benches/selection.rs and Cargo.toml
  • Check what changed since the last review
  • Post review

Nothing has changed since the last review. HEAD is still 2736d4c, the commit both the CodSpeed report and my earlier review were based on. Most of that review still applies: cache the fixtures in a LazyLock, add the MiMalloc global allocator like scalar.rs, replace the _ => fallback with an enum, give the bool args readable names, and note that retained_probes reads a single page (rows 8192..8448 with 1024-row pages). This pass corrects two points from that review and adds a few smaller ones. I still found no correctness bugs, and the untimed oracle checks are solid.

Corrections to the earlier review

  1. Point 8 (metadata_open_batch "decodes everything") was wrong. The 385 ms covers OPENS = 256 deserializations, which is about 1.5 ms per open. It is close to full[false] only by coincidence, so it does not show that deserialization decodes every page. The comment at selection.rs:269 is fine as written. If you want reports in per-open time, divide by OPENS the same way the module doc does for probes.
  2. Point 1 (budget) needs a caveat. CodSpeed's "CPU Simulation" numbers are simulated instruction cost, not wall time. They don't directly disprove the "tens of milliseconds on the baseline" target, which is a wall-time claim. The CI-time concern still holds, though. Each case compresses 4M rows outside the timer, with 40 cases, under valgrind. So caching the fixtures matters more than shrinking ROWS. Wall-time calibration on the target runner is still pending, as the PR body says.
  3. Point 3: CodSpeed reports 2 benchmarks with no measured execution time, not 1. The report doesn't say whether they come from this PR. Check the optimized_out filter before treating it as a blocker.

New, smaller points

  1. Magic numbers (selection.rs:128,132,137,140):
    • 4_194_304u64 repeats ROWS; use ROWS as u64.
    • 524288 appears three times; a const TAKE_LEN would make the shared workload size explicit.
    • The stride 32749 appears three times. It is odd, so it gives distinct indices mod 2²², which is worth a short comment and a named const.
  2. contiguous and near_full are a good pair. Both select the same rows, 1..ROWS-1: one through take, the other through slice. A short comment would stop someone from later "deduplicating" them as redundant.
  3. drop(actual) (selection.rs:252) does nothing useful. It drops a single Scalar that holds no meaningful untimed state, so it can go. The drop(check) / drop(reference) calls before the timed sections are reasonable.
  4. first_probes uses rows 8190 (null) and 8191 (valid). Both sit in page 7, and 8191 is the last row of that page. That's fine, but it ties into point 7 of the earlier review: retained probes use page 8 and first probes use page 7. If later PRs change page-boundary handling, a first probe that lands mid-page would be a less boundary-sensitive control.
  5. Timed take construction is intentional and fine. selection times both building the lazy take / slice and executing it. indices.clone() is just an Arc bump, and divan drops outputs outside the timer.

Checks

I only read the code. I did not run clippy, fmt or the benches; CLAUDE.md leaves verification to you, and the PR notes that scoped compile, lint and cloud calibration are still pending.

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

changelog/chore A trivial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant