Skip to content

refactor(array): pass the execution context into patch lookups - #10000

Draft
joseph-isaacs wants to merge 6 commits into
ji/patches-chunk-offset-probefrom
claude/context-passing-parent-fo4x8u
Draft

joseph-isaacs wants to merge 6 commits into
ji/patches-chunk-offset-probefrom
claude/context-passing-parent-fo4x8u

Conversation

@joseph-isaacs

Copy link
Copy Markdown
Contributor

Summary

Stacked on #9904. Follows up the review there by threading the caller's execution context through every patch lookup, rather than only into search_index_chunked.

Patches::get_patched and search_index each built their own execution context from the legacy session, even though every caller of get_patched is a scalar_at that already holds one.

Changes

  • Patches::get_patched, search_index, search_index_chunked and the private binary-search helpers take ctx: &mut ExecutionCtx instead of calling legacy_session().create_execution_ctx(). The clippy::disallowed_methods allowances on those paths go away.
  • The four scalar_at callers (ALP, ALP-RD, BitPacked, Sparse) pass their context through; two of them previously bound it as _ctx.
  • Patches::slice has no caller context, so it creates one legacy context at the top and reuses it for both index searches and the chunk base read.
  • The patches_lookup bench and the patches tests create a context once and pass it in.

API Changes

Patches::get_patched and Patches::search_index gain a ctx: &mut ExecutionCtx parameter.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ACe1mztjw9vNFUYwzVhGwz


Generated by Claude Code

joseph-isaacs and others added 6 commits September 16, 2026 09:58
`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.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UPw4v7SA2A3Ab5t9zwGbRV
…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.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UPw4v7SA2A3Ab5t9zwGbRV
`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.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UPw4v7SA2A3Ab5t9zwGbRV
…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.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UPw4v7SA2A3Ab5t9zwGbRV
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
`Patches::get_patched` and `search_index` each built their own execution
context from the legacy session, even though every caller of
`get_patched` is a `scalar_at` that already holds one. Thread the
caller's context through instead, down to the binary search over the
indices, so a patch lookup reads with the context it was invoked under
and builds none of its own.

`Patches::slice` has no caller context, so it creates one legacy context
and reuses it for both index searches and the chunk base read.

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
@codspeed

codspeed Bot commented Sep 23, 2026

Copy link
Copy Markdown

Merging this PR will regress 7 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.

⚠️ 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

⚡ 52 improved benchmarks
❌ 7 regressed benchmarks
✅ 112 untouched benchmarks
⏩ 2366 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
❌ filtered_sink_i64_avx512[OneNullInEight] 22.3 µs 31.6 µs -29.42%
❌ words_gather_dispatch_avx512[65536] 987 ns 1,343 ns -26.51%
❌ filtered_sink_i64_avx2[NineNullsInTen] 14.8 µs 18.2 µs -18.8%
❌ filtered_sink_i64_avx512[NineNullsInTen] 15 µs 18.3 µs -17.67%
❌ filtered_sink_i64_avx2[OneNullInEight] 26.1 µs 31.1 µs -15.99%
❌ multiply_shapes_neon[(16384, PerRowPerRow)] 17.4 µs 20.3 µs -14.09%
❌ words_gather_scalar_avx2[65536] 8.2 µs 9.4 µs -11.98%
⚡ mul_i16_nonnull_avx2 28.9 µs 11.6 µs ×2.5
⚡ mul_i32_nonnull_avx2 32.9 µs 13.4 µs ×2.5
⚡ mul_i8_nonnull_avx2 29.9 µs 12.6 µs ×2.4
⚡ mul_u8_nonnull_avx512 22.5 µs 9.6 µs ×2.3
⚡ mul_i32_nullable_avx2 34.4 µs 14.9 µs ×2.3
⚡ mul_i32_nonnull_neon 23.8 µs 10.4 µs ×2.3
⚡ mul_i8_nonnull_neon 25.7 µs 11.2 µs ×2.3
⚡ mul_i16_nonnull_neon 24.2 µs 10.6 µs ×2.3
⚡ mul_u8_nonnull_neon 19 µs 8.4 µs ×2.3
⚡ mul_u32_nonnull_neon 18.4 µs 8.1 µs ×2.3
⚡ mul_u64_nonnull_avx2 39.3 µs 17.4 µs ×2.3
⚡ mul_u16_nonnull_neon 18.4 µs 8.2 µs ×2.2
⚡ add_i32_nonnull_neon 17.4 µs 7.9 µs ×2.2
... ... ... ... ...

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

Tip

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


Comparing claude/context-passing-parent-fo4x8u (c550195) with ji/patches-chunk-offset-probe (2e0df78)

Open in CodSpeed

Footnotes

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

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

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant