perf: Implement native string lowering for Numba layouts and fix silent data corruption - #4250
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
|
Review of PR #4250 — perf: Implement native string lowering for Numba layoutsOverviewThe PR rewrites Main finding: this is a correctness fix, not only a perf changeThe old code called Verification performedAll checks below were run locally on macOS arm64 with Python 3.10 and Numba 0.64.0, loading this PR's
Performance: mixed resultBest of 5 runs in the same environment, 140,000 getitems inside
The ASCII string path regresses single-threaded: the decoder walks the bytes twice with a per-codepoint function call, while the old path was a memcpy (under the GIL). The genuine wins are correctness for non-ASCII, no GIL acquisition (which serialized Suggested (non-blocking) improvement: the first pass already computes Behavior changes to document
Public API changeNo. Both changed functions live in Existing tests modifiedNo. The diff contains exactly two files: Test coverageGood: all code-point widths, mixed-width strings, empty string/bytes, arbitrary bytes, and string operations. Missing: a test asserting Risks
Style nits (minor)
VerdictThe implementation is correct and leak-free under everything tested here, and it fixes real silent string corruption on top of the stated goal. Three changes are worth requesting before merge, none structural:
🤖 Generated with Claude Code |
TaiSakuma
left a comment
There was a problem hiding this comment.
Thank you.
I request these two of the three changes describied in #4250 (comment).
- Surface the correctness fix in the title or body (changelog visibility).
- Add invalid-UTF-8 tests.
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR4250 |
Thanks @TaiSakuma for a detailed review! Both requests are addressed: the title is fixed and the tests are added. |
|
I thought the title should be "fix:" rather than "perf:". I feel it generally makes more sense to use "fix:" if a PR fixes a bug and improves performance. But it is fine as is. I had AI look through the repo history and the Conventional Commits doc. There is no such precedence or ranking between types. |
I don’t mind one way or another. I marked it as performance based on the original issue, it turns out there was also a bug to fix 😀 |
Fixes #2704
Summary
This PR replaces the Python C API path used by
layout.py:string_numba_lowerwith a fully native implementation.The lowering now handles both:
"string"→ Numba nativeunicode_type"bytestring"→ Numba nativeBytesPreviously, reading strings from an Awkward Array inside
@numba.njitrequired acquiring the GIL, constructing temporary Pythonstr/bytesobjects, and boxing them back into Numba native values. This PR removes that round-trip entirely.The new implementation:
get_python_api,strorbytesobjects,unicode_typeandBytesvalues directly.For Unicode strings, the implementation decodes UTF-8 directly into Numba's native Unicode representation using Numba's internal
_empty_stringand_set_code_pointhelpers. For bytestrings, it allocates an NRT-managed buffer and copies the raw bytes directly.Motivation
This is the second step in removing Python C API dependencies from Awkward's Numba string support.
The previous PR #4249 implemented native lowering for appending Numba Unicode strings to an
ArrayBuilder. This PR completes the reverse direction by implementing native lowering for reading Awkward strings and bytestrings into Numba values.Besides removing unnecessary transitions between native and Python representations, this reduces GIL usage during JIT execution and makes the string lowering path fully native.
Tests
Added regression tests covering:
@numba.njitThese tests verify that strings and bytestrings can be retrieved from Awkward Arrays and used natively within compiled Numba functions without creating intermediate Python objects.