use digit run to determine float or integer decode - #286
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![macroscopeapp[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Merging this PR will regress 4 benchmarks
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
|
@codspeedbot explain this regression. How does this branch compare to |
Short versionTwo of the three flagged regressions aren't real — the code they'd have to come from is instruction-identical to Method: worktrees of The three flagged regressions
The regression that isn't flagged
The runner flamegraph shows why:
Three ways out, if you want to keep the design: have This branch vs
|
| document | main |
faster-floats |
this branch |
|---|---|---|---|
long_significand_floats.json |
506,658 | 515,129 (+1.7%) | 446,161 (-11.9%) |
bigints_array.json |
363,320 | 355,320 (-2.2%) | 296,597 (-18.4%) |
floats_array.json |
540,426 | 445,962 (-17.5%) | 481,725 (-10.9%) |
doubles_array.json |
587,171 | 482,223 (-17.9%) | 524,175 (-10.7%) |
short_numbers.json |
216,056 | 213,856 (-1.0%) | 235,486 (+9.0%) |
corpus floats |
83.32 M | 74.60 M (-10.5%) | 74.56 M (-10.5%) |
corpus ints |
241.2 M | 232.3 M (-3.7%) | 238.9 M (-1.0%) |
corpus error |
65.22 M | 64.62 M (-0.9%) | 66.00 M (+1.2%) |
string_array_unique.json |
2,332,965 | 2,332,965 | 2,332,965 |
Head-to-head on CodSpeed (walltime, aarch64) agrees on the shape: this branch wins long_significand_floats (+14.0% value, +15.3% iter), short_floats (+13.0% value), floats_array (+6.8%), bigints_array (+5.0%); faster-floats wins short_numbers (+6.6%), json_cases_error (+6.6%) and big_jiter_iter (+7.0%, though both are ahead of main there).
Read together: faster-floats is the stronger of the two on the plain double shape, because it feeds the mantissa it already scanned straight into Eisel-Lemire; it does nothing for long significands. This branch is the stronger one on long significands and big integers and matches it on the mixed corpus floats, but it charges every short integer for a scan it used to get for free. Fixing the fused decode for the ≤ 18-digit case would remove the one real regression here and make the two approaches complementary rather than alternatives.
No code changes were made — this was an analysis request.
Summary
- Warming up the performance wizard...
- Triage the 3 regressions on use digit run to determine float or integer decode #286 — Two of the three are layout drift (string path is instruction-identical); json_cases_error is ~1/3 real, and the genuine cost is the extra digit-run scan on short integers
- Compare dh/number-event with faster-floats — CodSpeed head-to-head plus local callgrind instruction counts on the same documents for main / faster-floats / this branch
- Write up findings — Analysis posted; no code changes were needed, nothing committed
- Handling comment — Comment handling finished successfully
Learnings updated. See what CodSpeed remembers about this repository.
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/jiter/src/simd/fallback_int.rs">
<violation number="1" location="crates/jiter/src/simd/fallback_int.rs:24">
P2: When `num-bigint` is enabled, negative integers with exactly 4,300 digits are rejected as out of range. Returning `None` here routes them through the scanner, whose limit includes the leading minus sign; base the scan limit on `digit_start` or otherwise exclude the sign from the 4,300-digit count.</violation>
<violation number="2" location="crates/jiter/src/simd/fallback_int.rs:24">
P2: For floats with 2+ fractional digits (e.g. `1.23`, `1.234`, `12.34`, `1.23e5`) the `None` check only tests `data.get(index + 3)`, so a digit in the fractional part triggers the digit-run scanner even though the integer part is shorter than four digits. These numbers now pay a SIMD `find_digit_run_end` scan plus a float-marker check before `parse_json_float`, where the previous code returned `IntChunk::Float` directly from `decode_int_chunk_limit::<4>`. Four-digit integers likewise re-scan and re-fold the leading digits. This extra work is a likely source of the regression you asked about; consider also checking `data.get(index+1)`/`data.get(index+2)` (or only returning `None` when all four bytes are digits) so short floats keep the direct `Float` path.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| /// at least four digits are present so the caller can scan the run from its SIMD-friendly start. | ||
| #[inline(always)] | ||
| pub(crate) fn decode_number_prefix(data: &[u8], index: usize) -> Option<(IntChunk, usize)> { | ||
| if data.get(index + 3).is_some_and(u8::is_ascii_digit) { |
There was a problem hiding this comment.
P2: When num-bigint is enabled, negative integers with exactly 4,300 digits are rejected as out of range. Returning None here routes them through the scanner, whose limit includes the leading minus sign; base the scan limit on digit_start or otherwise exclude the sign from the 4,300-digit count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/jiter/src/simd/fallback_int.rs, line 24:
<comment>When `num-bigint` is enabled, negative integers with exactly 4,300 digits are rejected as out of range. Returning `None` here routes them through the scanner, whose limit includes the leading minus sign; base the scan limit on `digit_start` or otherwise exclude the sign from the 4,300-digit count.</comment>
<file context>
@@ -17,23 +17,14 @@ pub(crate) fn find_digit_run_end(data: &[u8], mut index: usize, limit: usize) ->
- + u64::from(digits[3] & 0x0f);
- (IntChunk::Ongoing(value), index + 4)
+pub(crate) fn decode_number_prefix(data: &[u8], index: usize) -> Option<(IntChunk, usize)> {
+ if data.get(index + 3).is_some_and(u8::is_ascii_digit) {
+ None
} else {
</file context>
| /// at least four digits are present so the caller can scan the run from its SIMD-friendly start. | ||
| #[inline(always)] | ||
| pub(crate) fn decode_number_prefix(data: &[u8], index: usize) -> Option<(IntChunk, usize)> { | ||
| if data.get(index + 3).is_some_and(u8::is_ascii_digit) { |
There was a problem hiding this comment.
P2: For floats with 2+ fractional digits (e.g. 1.23, 1.234, 12.34, 1.23e5) the None check only tests data.get(index + 3), so a digit in the fractional part triggers the digit-run scanner even though the integer part is shorter than four digits. These numbers now pay a SIMD find_digit_run_end scan plus a float-marker check before parse_json_float, where the previous code returned IntChunk::Float directly from decode_int_chunk_limit::<4>. Four-digit integers likewise re-scan and re-fold the leading digits. This extra work is a likely source of the regression you asked about; consider also checking data.get(index+1)/data.get(index+2) (or only returning None when all four bytes are digits) so short floats keep the direct Float path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/jiter/src/simd/fallback_int.rs, line 24:
<comment>For floats with 2+ fractional digits (e.g. `1.23`, `1.234`, `12.34`, `1.23e5`) the `None` check only tests `data.get(index + 3)`, so a digit in the fractional part triggers the digit-run scanner even though the integer part is shorter than four digits. These numbers now pay a SIMD `find_digit_run_end` scan plus a float-marker check before `parse_json_float`, where the previous code returned `IntChunk::Float` directly from `decode_int_chunk_limit::<4>`. Four-digit integers likewise re-scan and re-fold the leading digits. This extra work is a likely source of the regression you asked about; consider also checking `data.get(index+1)`/`data.get(index+2)` (or only returning `None` when all four bytes are digits) so short floats keep the direct `Float` path.</comment>
<file context>
@@ -17,23 +17,14 @@ pub(crate) fn find_digit_run_end(data: &[u8], mut index: usize, limit: usize) ->
- + u64::from(digits[3] & 0x0f);
- (IntChunk::Ongoing(value), index + 4)
+pub(crate) fn decode_number_prefix(data: &[u8], index: usize) -> Option<(IntChunk, usize)> {
+ if data.get(index + 3).is_some_and(u8::is_ascii_digit) {
+ None
} else {
</file context>
| if data.get(index + 3).is_some_and(u8::is_ascii_digit) { | |
| if data.get(index + 3).is_some_and(u8::is_ascii_digit) | |
| && data.get(index + 1).is_some_and(u8::is_ascii_digit) | |
| && data.get(index + 2).is_some_and(u8::is_ascii_digit) | |
| { |
samuelcolvin
left a comment
samuelcolvin
left a comment
There was a problem hiding this comment.
generally LGTM, a few questions and needs a bit more docs.
| const JSON_FMT: u128 = lexical_format::JSON; | ||
|
|
||
| #[inline(always)] | ||
| fn parse_json_float(data: &[u8], start: usize, allow_inf_nan: bool) -> JsonResult<(f64, usize)> { |
There was a problem hiding this comment.
could do with a one-line docstring.
|
|
||
| #[cold] | ||
| #[inline(never)] | ||
| fn float_error(data: &[u8], start: usize, allow_inf_nan: bool) -> JsonResult<(f64, usize)> { |
| NumberInt::Int(value) | ||
| } | ||
|
|
||
| fn decode_integer_digits( |
| value *= match chunk_len { | ||
| 16 => 10u64.pow(16), | ||
| 18 => 10u64.pow(18), | ||
| _ => 10u64.pow(chunk_len as u32), | ||
| }; |
There was a problem hiding this comment.
is this really worthwhile? can we use ONGOING_CHUNK_MULTIPLIER?
|
|
||
| impl AbstractNumberDecoder for NumberAny { | ||
| fn decode(data: &[u8], index: usize, first: u8, allow_inf_nan: bool) -> JsonResult<(Self, usize)> { | ||
| fn decode(data: &[u8], mut index: usize, first: u8, allow_inf_nan: bool) -> JsonResult<(Self, usize)> { |
There was a problem hiding this comment.
can we add a docstring here explaining the general approach and rationale.
2 participants
Possible alternative to #281
Summary by cubic
Refactors number decoding to scan the digit run first, then decide between integer and float paths. This avoids a speculative float parse for integers and simplifies error handling by eliminating a re-parse fallback.
Changed
find_digit_run_endSIMD helpers for x86_64 and aarch64 to locate the end of digit runs quickly.NumberAnydecoding by treatingInfinityandNaNas float inputs rather than separate enum variants.Written for commit 87f0ec0. Summary will update on new commits.