Repository navigation
Reduce monomorphization in sequence and run-end take - #11102
Open
robert3005 wants to merge 7 commits into
Open
robert3005 wants to merge 7 commits into
robert3005 wants to merge 7 commits into
Conversation
Merging this PR will improve performance by 12.07%1 warning
Performance Changes
Tip Curious why performance improved? Comment Comparing 
Footnotes
|
robert3005
enabled auto-merge (squash)
November 2, 2026 16:32
connortsui20
reviewed
Nov 2, 2026
connortsui20
reviewed
Nov 2, 2026
connortsui20
disabled auto-merge
November 2, 2026 16:42
connortsui20
enabled auto-merge (squash)
November 2, 2026 19:22
connortsui20
added a commit
that referenced
this pull request
Nov 2, 2026
Short walltime benchmarks can report changes on unchanged code because CodSpeed compares the fastest sample. Batch the bitpacked filter and take samples, and keep unreliable cases local. Exclusions live beside the benchmark definitions, using `#[cpu_features(exclude = ["neon"])]` for whole CPU variants and source-level selection for individual inputs. CI commands stay generic. Move the chunked-take family to walltime with 16 iterations per sample so it measures the operation's allocator and syscall costs. Keep its x86 variants local after repeatability checks failed; all 27 NEON cases remain in CI. The guide retains practical examples and measurement guidance without an exclusion catalog or a repeatability tool. <details> <summary>Measurement evidence</summary> Spread is `max / min - 1` across independent processes, with 1000 Divan samples per process and ten processes on each of three matching hosts. | Case | Before | Fixed sampling | | --- | --- | --- | | AVX2 `filter[80]`, sample size 64 | 9.36% | 1.45% | | AVX-512 `threshold[80]`, sample size 16 | 8.26% | 1.69% | Across independent base and head AVX2 builds, the fixed filter's worst spread was 4.66% over 60 processes. AVX-512 filter cases stayed within 1.78%. The excluded NEON string case still varied by 6.38%. Measurements used existing CI executables with matching sample-size overrides from [run 41449104113](https://github.com/vortex-data/vortex/actions/runs/41449104113) (`44bdefff6e69`) and [run 41442182591](https://github.com/vortex-data/vortex/actions/runs/41442182591) (`7c9bb58e39fe`), on three `c7i.metal-24xl` hosts for x86 and three `c7g.metal` hosts for NEON. These measure repeatability, not kernel speedups. Other Rust exclusions follow unexplained variation recorded in [#10808](#10808) and repeated on the workflow-only [#11095](#11095 (comment)). The two Python integer scalar scans include variable calling-thread reactor polling, visible in [#11102](#11102), even with memory mapping and background workers disabled. The chunked-take warning on `dfb32c5`, compared with `d8e7467`, came from mimalloc lazily allocating page-map metadata. The affected `small_m/shuffled/primitive/nonnull/chunks=2048/indices=16` case made one `mmap` and one `prctl` call totaling 23.3 µs; the base trace made neither. An untimed warm-up removed the syscall warning but produced eight regression alerts with unchanged kernel code under allocation-excluded simulation. Walltime measures elapsed time without that allocation exclusion and establishes separate per-CPU baselines. Walltime repeats used the CI executables from `29920deef0`, 1000 samples per process, and ten independent processes on each of three matching hosts per architecture. All 27 NEON cases stayed within 4.35% spread; the originally flagged case stayed within 2.88%. The worst x86 spreads were 11.04% for AVX2 and 11.20% for AVX-512. Increasing the sample size to 64 on two troublesome x86 cases still produced 12.15% and 13.38% spread, so those CPU variants remain local through the macro exclusion. </details> Focused native and simulation Clippy checks passed. Test-mode runs confirmed the local, AVX2, and NEON string case selection and the macro's NEON exclusion. Pinned-nightly formatting, Ruff, ty, YAML linting, and patch whitespace checks passed. All 66 CI checks passed on `29920deef0`, including CodSpeed analysis with zero syscall warnings. The final x86 exclusion passed focused native and simulation Clippy checks and test-mode checks for both excluded variants. CI is rerunning for that final change. Local Git hooks were bypassed. AI assistance: Codex helped investigate, implement and prepare this PR. --------- Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Read take indices through their unsigned reinterpretation, matching the varbin and list take kernels, so signed index types no longer produce separate copies of the kernels. The sequence take kernel also evaluates in the unsigned counterpart of the sequence ptype, since wrapping arithmetic produces the same bits in either signedness, and reinterprets the result back. This cuts it from 64 instantiations to 16. The run-end take kernel drops from 8 to 4 index widths in TakeExecute. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SWbEvHomkqAcFrW6vHohG Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
take_inner had grown to eight arguments, tripping clippy's too_many_arguments lint. It now derives base, multiplier, ptype and length from the sequence view itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SWbEvHomkqAcFrW6vHohG Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
Negative indices of every signed width are rejected as out of bounds, and a negative value under a null index is ignored. The take tests used an empty session, so the Sequence take kernel was never registered and every case, including the conformance suite, ran through the canonical primitive take. Initialize the session so the tests exercise the Sequence kernel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SWbEvHomkqAcFrW6vHohG Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
Draws ascending and descending sequences of every integer type with negative and positive bases, takes with indices of every integer type, and compares against an exact i128 evaluation of base + index * multiplier. Null indices hold negative or out-of-bounds values that the kernel must ignore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SWbEvHomkqAcFrW6vHohG Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
robert3005
force-pushed
the
claude/great-mccarthy-5j1hkt
branch
from
November 5, 2026 13:08
1eb9b44 to
7815d13
Compare
myrrc
reviewed
Nov 6, 2026
myrrc
reviewed
Nov 6, 2026
myrrc
reviewed
Nov 6, 2026
Trim the doc comment of take_inner. Test negative indices with one index type, because the check does not depend on the type. Signed-off-by: Robert Kruszewski <github@robertk.io>
Replace the exact i128 model with a check against a take from the canonical values of a generated sequence. Signed-off-by: Robert Kruszewski <github@robertk.io>
myrrc
reviewed
Nov 6, 2026
Draw the base, multiplier and length freely and reject sequences that do not fit the type, so that the test states only the property. Signed-off-by: Robert Kruszewski <github@robertk.io>
This branch has not been deployed
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.
Read take indices through their unsigned reinterpretation, matching the varbin and list take kernels, so signed index types no longer produce separate copies of the kernels.
The sequence take kernel also evaluates in the unsigned counterpart of the sequence ptype, since wrapping arithmetic produces the same bits in either signedness, and reinterprets the result back. This cuts it from 64 instantiations to 16. The run-end take kernel drops from 8 to 4 index widths in TakeExecute.