PERF: recover read_csv string-column speed after GH-66415 - #66426
Draft
jbrockmendel wants to merge 3 commits into
Draft
PERF: recover read_csv string-column speed after GH-66415#66426jbrockmendel wants to merge 3 commits into
jbrockmendel wants to merge 3 commits into
Conversation
jbrockmendel
marked this pull request as ready for review
July 27, 2026 22:13
jbrockmendel
marked this pull request as draft
July 28, 2026 00:21
…short tokens GH#66415 swapped strlen for _token_len boundary arithmetic in _string_pyarrow_utf8 pass 1, which put the boundary load's cache miss on the offsets dependency chain and lost the scan's prefetch of the bytes pass 2 copies; bandwidth-bound parallel reads paid ~20% on short-token string columns. Add _token_len_checked: boundary-first, strlen for tokens <= 32 bytes with a one-compare embedded-NUL verify, boundary arithmetic for longer tokens (which want the O(1) form). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Supersedes the _token_len_checked helper from the previous commit, which measured 11.6% slower than main on short-token string columns. GH#66415 computed pass-1 token lengths from word_starts, a second metadata array streamed alongside the words array coliter_next already reads. Taking the boundary from words is identical arithmetic over one array. 123-case csvbench sweep vs main: median 1.0018, 30 cases >=1% faster (top 1.113x), no confirmed regression. Also points the embedded-NUL test at GH#19886, the issue it actually covers, and drops an unsound dtype=object oracle: the C object path interns words by their NUL-terminated prefix, so it is not a valid reference for NUL-bearing data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment-only follow-ups from reviewing the branch: - The embedded-NUL comment and test cited GH#66277, the perf issue, and briefly GH#19886, which this branch does not fix (three of that issue's four reported cases still fail). Both now cite GH#66415, matching the whatsnew entry for the fix. - The helper's precondition named coliter_next; its only caller uses coliter_next_with_idx, so the precondition is now checkable. - The test comment explained the wrong thing about the two columns: "a" covers the words[token_idx + 1] branch and "b" the stream-end branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jbrockmendel
force-pushed
the
perf-read_csv-strlen-depchain
branch
from
July 29, 2026 14:53
72f2d91 to
3faea86
Compare
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.
Follow-up to GH-66415. That PR fixed read_csv's c engine truncating strings at an embedded NUL by replacing
strlenwith_token_lenin_string_pyarrow_utf8pass 1._token_lenreadsword_starts— a second per-token metadata array, streamed alongside thewordsarray thatcoliter_next_with_idxalready reads — which cost up to 12% on string columns._token_len_wordstakes the boundary fromwordsinstead. The tokenizer maintainswords[i] == stream + word_starts[i]at every mutation site, so the arithmetic is identical; what changes is that the loop touches one metadata array instead of two.No whatsnew:
_string_pyarrow_utf8arrived in 3.1.0-dev via GH-65283, so neither the regression nor this fix has shipped.Benchmarks
Full 123-case shared read_csv suite, M3 Pro, branch
3faea86vs a baseline worktree pinned at9828540. Adjacent-paired, adaptive 8–40 rounds to a ±0.75% target, 99% CIs, 1% effect floor. A separate worktree pinned at the same commit provides the null control column. Both build artifacts hash-verified unchanged across the run; 0 cases skipped.Median 1.0010. 18 cases ≥3% faster, 17 more 1–3% faster, no regression ≥3%.
str @6str dtype_backend=pyarrow @12str 6-char high-card [par]str @12str low_memory [par]str dtype_backend=pyarrowhigh-card strings @12str [par]quoted @6high-card strings @6high-card pyarrow [par]high-card strings [par]quoted [par]datetime-as-strescaped quotes @6parse_datesnon-ASCII UTF-8 [par]str @2Two small regressions
Both clear the significance bar, and the null control reproduces neither, so neither is machine noise.
int64, 3 mixed-magnitude cols [par]high-card strings @2int_mixed_magis three pure-integer columns, so_string_pyarrow_utf8never runs for it and the changed code cannot be executing. The plausible mechanism is code layout: this PR adds a function toparsers.pyxand shifts everything after it. A base-vs-base null control cannot detect that, since identical source produces identical layout — so the null clearing this case is weaker evidence than it looks.high-card strings @2is a genuine narrow regression: the same fixture wins at 1, 6 and 12 threads and under[par](1.022x / 1.098x / 1.114x / 1.071x), and loses only at 2. Unexplained.One further case,
medium (~6 MB) @2at 1.016x, is classified an artifact — the null control returns 1.013x on identical code, so it is that fixture's own instability rather than an effect of the branch.Variants tried and dropped
Noted so they are not re-proposed. A hybrid helper using
strlenunder a 32-byte cap plus a one-compare embedded-NUL verify cost 11.6% on plainstrand 10% onstr @2: token length does not separate the cases it was meant to separate, sincestris 4–7 byte tokens andstr_nonascii16–19, both under the cap. A prefetch variant, testing whetherstrlenwon by warming the lines pass 2 copies, came out statistically indistinguishable from this version across all 19 string cases.