Skip to content

Optimize SearchSorted to reuse probes and avoid repeated context creation - #9904

Open
joseph-isaacs wants to merge 6 commits into
developfrom
ji/patches-chunk-offset-probe
Open

joseph-isaacs wants to merge 6 commits into
developfrom
ji/patches-chunk-offset-probe

Conversation

@joseph-isaacs

Copy link
Copy Markdown
Contributor

Summary

This change optimizes the SearchSorted implementations to reuse a single RepeatedArrayProbe and execution context across all comparisons in a search, rather than creating a new probe and context for each element access. This significantly reduces overhead for searches over encoded arrays.

Additionally, a new SearchSortedArray adapter is introduced to support searching over arrays of any encoding using Scalar comparisons, with the same probe-reuse optimization.

Changes

SearchSortedPrimitiveArray Optimization

  • Refactored SearchSortedPrimitiveArray from a tuple struct to a struct with named fields (reader, len, ctx, _ptype)
  • Introduced a Reader<'a, T> enum that distinguishes between two cases:
    • Values(&'a [T]): Direct buffer access for canonical, non-nullable, host-backed arrays (zero-copy fast path)
    • Probe(RefCell<RepeatedArrayProbe>): Probe-based access for all other cases
  • Added reader() method to determine which access pattern to use based on array properties
  • Added typed_value() method that returns Option<T> to distinguish nulls from zero values
  • Simplified value() method to use typed_value() and map nulls to T::zero()
  • Updated IndexOrd<Option<T>> implementation to use a single typed_value() call instead of separate validity and value checks
  • All field accesses now use the struct fields instead of tuple indexing

New SearchSortedArray Implementation

  • Created new scalar.rs module with SearchSortedArray struct for searching any array type using Scalar comparisons
  • Implements IndexOrd<Scalar> with probe reuse across the entire search
  • Includes tests demonstrating scalar search with and without nulls

Module Organization

  • Exported SearchSortedArray from search_sorted/mod.rs
  • Removed the generic IndexOrd<Scalar> implementation for ArrayRef (now handled by SearchSortedArray)

Patches Optimization

  • Refactored search_index_chunked() to reuse a single probe and context for three reads of chunk_offsets, rather than creating a new pair per read
  • Added #[allow(clippy::disallowed_methods)] annotation to document the intentional use of legacy_session()

Test Coverage

  • Added search_sorted_reads_canonical_values_directly() test verifying the fast path for canonical arrays
  • Added search_sorted_probes_when_values_are_not_addressable() test verifying probe-based access for nullable arrays
  • Added search_sorted_scalar() and search_sorted_scalar_with_nulls() tests for the new SearchSortedArray
  • Updated fuzz tests to pass execution context to assert_search_sorted()

API Changes

The SearchSortedPrimitiveArray struct layout changed from a tuple struct to a named-field struct. This is a breaking change for any code that directly constructs or pattern-matches on this type, though it is primarily used through the SearchSorted trait.

A new public type SearchSortedArray is introduced for searching arrays with Scalar comparisons.

https://claude.ai/code/session_01UPw4v7SA2A3Ab5t9zwGbRV

@codspeed

codspeed Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Merging this PR will regress 5 benchmarks

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ 15 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 6 improved benchmarks
❌ 5 regressed benchmarks
✅ 2172 untouched benchmarks
⏩ 359 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ WallTime mul_u64_nonnull_neon 28.7 µs 40 µs -28.15%
❌ WallTime multiply_shapes_neon[(32768, PerRowPerRow)] 32.7 µs 38.2 µs -14.47%
❌ WallTime mul_i64_nonnull_neon 32.7 µs 38.2 µs -14.33%
❌ ⚠️ Simulation cold[(16, 64)] 311.4 µs 355.3 µs -12.38%
❌ WallTime bitpack_blocked_compress_avx2 6.7 µs 7.6 µs -11.35%
⚡ ⚠️ Simulation density_sweep_dense_runs[0.001] 47.8 µs 29.3 µs +63.4%
⚡ Simulation search_index_mixed_out_of_range_chunked 330.9 µs 225.6 µs +46.66%
⚡ Simulation search_index_below_min_chunked 332.5 µs 227.1 µs +46.42%
⚡ Simulation search_index_full_range_random_chunked 362.9 µs 257.7 µs +40.81%
⚡ Simulation search_index_above_max_chunked 459.6 µs 354.2 µs +29.75%
⚡ Simulation search_index_in_range_chunked 460.9 µs 355.7 µs +29.58%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ji/patches-chunk-offset-probe (0f919f0) with develop (731a231)

Open in CodSpeed

Footnotes

  1. 359 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. ↩

Comment thread vortex-array/src/patches.rs Outdated
Comment thread vortex-array/src/patches.rs Outdated

@robert3005 robert3005 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.

let's merge this, just one comment about ctx

@robert3005
robert3005 force-pushed the ji/patches-chunk-offset-probe branch from 2e0df78 to e7a8cf1 Compare September 24, 2026 21:08
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days

@github-actions github-actions Bot added the stale This PR is stale and will be auto-closed soon label Oct 10, 2026
@robert3005 robert3005 removed the stale This PR is stale and will be auto-closed soon label Oct 10, 2026
joseph-isaacs and others added 6 commits October 10, 2026 13:40
`SearchSortedPrimitiveArray` held a bare `&ArrayRef` and did a one-off
`execute_scalar` per comparison, so every probe in a binary search rebuilt
the state its predecessor had just thrown away. For a nullable array that
meant resolving `Validity` from the encoding on each read, and
`IndexOrd<Option<T>>` paid for it twice: once in its own `is_valid` call and
again inside `execute_scalar`, which checks validity before dispatching.

Hold a `RepeatedArrayProbe` instead. Validity is resolved on the first
comparison and reused by the remaining ~log2(n), as is any state the
encoding keeps. A null element reads back as a null scalar, so the separate
`is_valid` call is redundant: `IndexOrd<Option<T>>` now decides from the one
read, halving the probes on the nullable path.

The array is no longer borrowed for the searcher's lifetime, since the probe
owns a handle to it; only the execution context is.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UPw4v7SA2A3Ab5t9zwGbRV
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
…rray

`Patches::search_index` already sidesteps the scalar path when its indices
are a canonical primitive array, searching the buffer as a `&[T]`.
`RunEnd::find_physical_index` has no such path, so a run-end array whose
ends are plain `u32`s — which is what they are after a file read — still
pays a scalar read per probe.

Put the fast path in the searcher instead of in each caller, so every
`SearchSortedPrimitiveArray` user gets it. The buffer can only be read as
`[T]` when the array is canonical, is host-backed, and is non-nullable,
since a null element leaves an arbitrary value in the buffer; everything
else keeps the probe.

The array is borrowed for the searcher's lifetime again, as the values are.

This also settles the cost the previous commit adds on non-nullable arrays,
where a retained read buys nothing while no encoding keeps state: those
arrays no longer reach the probe at all. What still does — compressed ends,
non-primitive patch indices — is where a retained probe pays off once those
encodings keep state.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UPw4v7SA2A3Ab5t9zwGbRV
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
`IndexOrd<Scalar> for ArrayRef` built a whole execution context per
comparison — `legacy_session().create_execution_ctx()` inside `index_cmp` —
so a binary search over 65,536 elements created sixteen of them and threw
each away after one read. A trait impl on `ArrayRef` has nowhere to keep
anything, so this could not be fixed in place.

Replace it with `SearchSortedArray`, which takes the caller's context and a
`RepeatedArrayProbe`, matching `SearchSortedPrimitiveArray`. The searcher
now has the same shape whether or not the element type is known, and the
doc points at the typed one, which reads canonical values directly.

The only caller was the fuzzer, which already had a context in scope.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UPw4v7SA2A3Ab5t9zwGbRV
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
…eads

`Patches::search_index_chunked` read three chunk offsets out of the same
array through `chunk_offset_at`, which builds an execution context and a
one-off probe per call — three of each per lookup, for three reads of one
small array.

Read them through a single `RepeatedArrayProbe` and a single context.

`chunk_offset_at` stays as the public single-read accessor.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UPw4v7SA2A3Ab5t9zwGbRV
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Take the context as a parameter instead of building one from the legacy
session inside the method. `search_index` builds it once for the chunked
path.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ACe1mztjw9vNFUYwzVhGwz
`get_patched` and `search_index` now take `ctx: &mut ExecutionCtx` instead
of building a legacy context per call, and the binary search helpers thread
it through to `SearchSortedPrimitiveArray`. The `scalar_at` callers in ALP,
ALP-RD, BitPacked and Sparse pass their context along. `Patches::slice` has
no caller context, so it builds one and reuses it for both searches and the
chunk base read.

Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LiW5BN1gBGNfrsVf4fDoMq
@robert3005
robert3005 force-pushed the ji/patches-chunk-offset-probe branch from e7a8cf1 to 0f919f0 Compare October 10, 2026 12:40
@robert3005
robert3005 marked this pull request as ready for review October 10, 2026 12:41

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/performance A performance improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants