refactor(array): pass the execution context into patch lookups - #10000
joseph-isaacs wants to merge 6 commits into
Conversation
`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
Merging this PR will regress 7 benchmarks
|
| 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)
Footnotes
-
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. ↩
2e0df78 to
e7a8cf1
Compare
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_patchedandsearch_indexeach built their own execution context from the legacy session, even though every caller ofget_patchedis ascalar_atthat already holds one.Changes
Patches::get_patched,search_index,search_index_chunkedand the private binary-search helpers takectx: &mut ExecutionCtxinstead of callinglegacy_session().create_execution_ctx(). Theclippy::disallowed_methodsallowances on those paths go away.scalar_atcallers (ALP, ALP-RD, BitPacked, Sparse) pass their context through; two of them previously bound it as_ctx.Patches::slicehas no caller context, so it creates one legacy context at the top and reuses it for both index searches and the chunk base read.patches_lookupbench and the patches tests create a context once and pass it in.API Changes
Patches::get_patchedandPatches::search_indexgain actx: &mut ExecutionCtxparameter.🤖 Generated with Claude Code
https://claude.ai/code/session_01ACe1mztjw9vNFUYwzVhGwz
Generated by Claude Code