perf: widen fast_float_strtod fast path to 17-19 digit mantissas#15061
Conversation
🤖 Augment PR SummarySummary: Extends Key changes:
🤖 Was this summary useful? React with 👍 or 👎 |
| if (exponent >= 0) { | ||
| /* (mantissa * 10^e) fits in 128 bits. Convert exactly: the | ||
| * single (double) cast from __uint128_t rounds to nearest. */ | ||
| __uint128_t prod = (__uint128_t)mantissa * (uint64_t)powers_of_ten[exponent]; |
There was a problem hiding this comment.
src/fast_float_strtod.c:288: The comment says the __uint128_t product is converted with a “single (double) cast”, but the implementation decomposes into hi/lo and does multiple casts plus an addition. Consider clarifying the rationale/proof that this decomposition is still guaranteed to match strtod() across the widened-fast-path input space.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| * 16-19 significant digit case. */ | ||
| uint64_t divisor = (uint64_t)powers_of_ten[-exponent]; | ||
| __uint128_t scaled = (__uint128_t)mantissa << 64; | ||
| __uint128_t q = scaled / divisor; |
There was a problem hiding this comment.
src/fast_float_strtod.c:304: This negative-exponent widened path uses truncated integer division (scaled / divisor) without looking at the remainder, while the comment claims bit-exact IEEE rounding “by construction”. For values close to a rounding midpoint this could bias results downward unless there’s a proven error bound; consider documenting that bound or adding a tie-case test.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| /* 17-19 significant digit mantissas — negative exponent (scores in [0,1)) */ | ||
| {"0.49606648747577575", 0.49606648747577575}, /* 17 sig digits, ZADD hot case */ | ||
| {"0.8731899671198792", 0.8731899671198792}, /* 16 sig digits */ |
There was a problem hiding this comment.
src/fast_float_strtod.c:489: This block is labeled as covering the widened mantissa > 2^53 / __uint128_t path, but inputs like 0.8731899671198792 (and 0.999999999999999) have mantissas ≤ 2^53 so they’ll likely exercise the original Clinger branch instead. That reduces coverage of the new path and may hide regressions.
Severity: low
Other Locations
src/fast_float_strtod.c:493tests/unit/type/zset.tcl:1775
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| uint64_t hi = (uint64_t)(q >> 64); | ||
| uint64_t lo = (uint64_t)q; | ||
| value = ((double)hi * 18446744073709551616.0 + (double)lo) | ||
| * 5.421010862427522170037e-20; /* 2^-64 */ |
There was a problem hiding this comment.
Hi-lo split double rounding produces 1-ULP incorrect results
Medium Severity
The hi-lo split (double)hi * 2^64 + (double)lo does not always produce a correctly-rounded double for the 128-bit integer q. When hi exceeds 2^53 and (double)hi rounds down (ties-to-even), the lo component is too small relative to the double's ULP at that scale to compensate, so it gets absorbed by the IEEE addition. This causes 1-ULP errors versus strtod. For example, input "9007199254740993.5" returns 9007199254740992.0 instead of the correct 9007199254740994.0. The pattern triggers whenever hi is an odd integer just above 2^53 (causing a round-down) and lo > 0. The same issue affects both the positive and negative exponent widened paths.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 82e82a5. Configure here.
Recovers the regression on float-parsing-heavy sorted-set listpack workloads introduced when the C++ fast_float library was replaced by the pure-C implementation. Bisect on `memtier_benchmark-1Mkeys-load-zset-listpack-with-100-elements-double-score` (x86-aws-m7i.metal-24xl / arm-aws-m8g.metal-24xl, n=3 datapoints each): | Commit | x86 ops/sec | ARM ops/sec | |--------------------------------------|------------:|------------:| | 8.6 branch (a176d12) | 5,633 | 6,502 | | Right before the rewrite | 5,617 | TBD | | Current unstable (with the rewrite) | 5,310 | 4,721 | | **After this patch (x86 measured)** | **9,788** | TBD | x86 result: +73.8% vs 8.6, +84.3% vs unstable. ARM measurement pending. ## Root cause ~50% of random double scores have 17-19 significant digits, which exceed MAX_MANTISSA_FAST_PATH (2^53). These fell through to the strtod() fallback (memcpy + null-term + glibc strtod), which on ARM is significantly slower than the fast path and is essentially the entire regression vs 8.6. ## Fix Compute (mantissa * 10^exp) in 128-bit integer arithmetic via `__uint128_t` and convert to double with a single IEEE round-to- nearest-even cast. Supported for |exp| in [0, 19] where 10^|exp| fits in uint64. Cases outside that range (or truly corrupt inputs) still fall through to strtod(). ## Correctness Bit-exact with strtod on every tested case: - `src/fast_float_strtod.c` unit tests: 98 passed (77 original + 21 new), 0 failed. New cases exercise positive/negative exponents, mantissa exactly at 2^53 boundary, ties-to-even rounding, 19-digit mantissas, cases that fall back to strtod. - `tests/unit/type/zset.tcl` integration tests: round-trip ZADD → DEBUG RELOAD → ZSCORE on listpack + skiplist encodings with 17-19 significant digit scores. ## Code delta ~50 LOC in fast_float_strtod.c, ~25 LOC of unit tests, ~30 LOC of TCL integration test. No new dependencies; pure C; no libstdc++ reintroduced. ## References The fast-path concept is Clinger's, the pure-C impl being extended is antirez/sundb/mpozniak95/moticless's, the broader algorithmic context is fast_float (Lemire & Magalhães). None of those directly cover the narrow 'widen the Clinger fast path via __uint128_t one-shot' trick this patch uses — that was written for this patch. - Clinger, 'How to Read Floating Point Numbers Accurately' (PLDI 1990) — the existing pure-C fast path is a direct application of Clinger; this patch only widens its precondition. - Pure-C fast_float_strtod.c being extended: redis#14661 by @antirez, @sundb, @mpozniak95, @moticless. The existing Clinger fast path (mantissa <= 2^53) is unchanged. - Related reading: Lemire & Magalhães, 'Number Parsing at a Gigabyte per Second' (2021). A full port via the Eisel-Lemire algorithm (see github.com/kolemannix/ffc.h, ~3.5k LOC) achieves correct rounding over the full exponent range. This patch takes the narrower path that only needs ~50 LOC to cover the Redis use case.
82e82a5 to
7de270d
Compare
Co-authored-by: debing.sun <debing.sun@redis.com>
@moticless post in the wrong place. |
Per sundb's review comment on PR redis#15061: https://github.com/redis/redis/pull/15061/files#r3099614799
Co-authored-by: debing.sun <debing.sun@redis.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit d0ad649. Configure here.
Without `set i 0` / `incr i`, the `foreach` loop reuses a stale `$i` from the preceding ZSCORE test (equal to `$elements`), so every ZADD writes the same member and the test no longer validates round-trip correctness for multiple 17-19 significant-digit mantissas. After `debug reload` only the last score is stored, and the post-reload assertions would not detect a regression in any mantissa other than the last one.
|
Regarding the comment: #14661 (comment) I’ve tested this locally using the Redis unit test suite and found that the PR might fail on specific rounding-boundary cases in the widened fast path. To reproduce, you can add these cases to fast_float_strtod.c at line 527: Then run the test using:
|
) ## Root cause Roughly 50% of random double scores generated by the ZADD listpack workload have 17-19 significant digits, which exceed `MAX_MANTISSA_FAST_PATH` (`2^53`). These inputs fall through to the `strtod()` fallback: ```c char static_buf[128]; memcpy(buf, nptr, len); /* memcpy back! */ buf[len] = '\0'; /* null-term */ double result = strtod(buf, ...); /* glibc strtod — ~10× slower on ARM */ ``` The original C++ `fast_float` library handled the same 17-19 digit inputs with Eisel-Lemire / bigint arithmetic without falling back to `strtod()`. That is what the pure-C replacement lost. ## Fix Compute `mantissa * 10^exponent` in 128-bit integer arithmetic using `__uint128_t`, then convert to double with a single IEEE round-to-nearest-even cast. Supported for `|exp| in [0, 19]` where `10^|exp|` fits in `uint64`; cases outside that range (or otherwise outside the fast path's preconditions) still fall through to `strtod()`. --------- Co-authored-by: debing.sun <debing.sun@redis.com>
…15111) ## Summary Fixes a ±1 ULP rounding mismatch between `fast_float_strtod()` and libc `strtod()` in the widened (mantissa > 2^53) fast path introduced by #15061. Reported by @vitahlin in #14661 (comment) with two minimal reproducers: ``` input: 9007199255094284e-19 fast_float_strtod: 0x3f4d83c94fbbcb8a libc strtod : 0x3f4d83c94fbbcb8b delta -1 ULP input: 2489830482329185244e1 fast_float_strtod: 0x43f59888c51e5b4c libc strtod : 0x43f59888c51e5b4b delta +1 ULP ``` Redis treats `strtod()` as the fallback for `fast_float_strtod()`, so every fast-path-accepted input is contractually expected to be bit-exact with `strtod()`. The two cases above are accepted by the widened branch but produce a different IEEE-754 representation, breaking the contract. ## Root cause The widened branch added in #15061 used a homebrew shortcut to convert a 128-bit integer product to a double: ```c value = (double)hi * 18446744073709551616.0 + (double)lo; ``` This is **not** a single-rounding operation — `(double)hi` rounds when `hi > 2^53`, and the subsequent `+ (double)lo` rounds again. For inputs near the round-half-to-even boundary, the two roundings can compose into the wrong direction. The negative-exponent branch is doubly affected: integer division `scaled / divisor` truncates the remainder before the conversion, so even a hypothetical correct `hi*2^64+lo` step would round down on inputs that should round up. ## Fix Replace the homebrew widened branch with the **Eisel-Lemire** algorithm from upstream `fast_float`. This is the same algorithm that `fast_float`'s own widened path uses; bit-exact-with-strtod for inputs with ≤19-digit mantissa is proved in: > Noble Mushtak and Daniel Lemire, "Fast Number Parsing Without Fallback". Pieces ported (MIT-licensed, from `fast_float/include/fast_float/fast_table.h`, `decimal_to_binary.h`, `float_common.h`): - 128-bit precomputed extended-precision powers of five (`5^-342 ... 5^308`, 651 entries) — pure data. - `compute_product_approximation` — 128-bit multiply with high-half rounding-boundary fixup. - `compute_float` — the main algorithm; returns `(mantissa, power2)` ready to be packed. - `am_to_double` — IEEE-754 binary64 bit-pack. - `__builtin_clzll` and `__uint128_t` wrappers (with a 32-bit fallback for portability). Clinger's strict fast path (`mantissa ≤ 2^53` and `|exp| ≤ 22`) is **kept** unchanged — it's a single double multiply/divide and is faster than Eisel-Lemire on its domain. Only the buggy widened branch is replaced. The port stays minimal: - **double-only** (no `float`, no `long double`) - No bigint slow path. The rare "indeterminate" inputs that upstream resolves with `digit_comp` are unreachable from `parse_number_string`'s ≤19-digit mantissa per the Mushtak-Lemire proof, but a defensive `am.power2 < 0` check is preserved that falls back to libc `strtod()` if any future caller widens the input domain. ## Why not just revert #15061? Considered. Reverting restores correctness at the cost of the +73-84 % zset listpack-load wins #15061 measured on 17-19 digit double scores. Eisel-Lemire is *the* algorithm that gives both correctness *and* the wider mantissa range — preserving #15061's wins while fixing the rounding regression. A "tightened admission filter" (only accept widened-path inputs where the conversion happens to be single-rounding) was also considered. The math shows the filter conditions are essentially unsatisfied for typical inputs (`lo == 0` requires the 128-bit product be divisible by 2^64; only ~1 in 10^13 random inputs qualify), making it equivalent to a revert with extra dead code. Eisel-Lemire is the only widened-path solution that preserves perf on the typical case.



Motivation
Bisect (2026-04-16) on
memtier_benchmark-1Mkeys-load-zset-listpack-with-100-elements-double-score(x86-aws-m7i.metal-24xl / arm-aws-m8g.metal-24xl, n=3 datapoints per commit) localized a regression on this benchmark to the commit that replaced the C++fast_floatlibrary with the pure-C implementation:a176d128.6 branch (8.6.2)3cd4642right before the rewrite2f1a8b2right after the rewrite3bcfbbecurrent unstableAll other commits between 8.6 and
3cd4642are flat vs 8.6 — the single commit that rewritesfast_float_strtodis 100% of the regression on this workload.Root cause
Roughly 50% of random double scores generated by the ZADD listpack workload have 17-19 significant digits, which exceed
MAX_MANTISSA_FAST_PATH(2^53). These inputs fall through to thestrtod()fallback:The original C++
fast_floatlibrary handled the same 17-19 digit inputs with Eisel-Lemire / bigint arithmetic without falling back tostrtod(). That is what the pure-C replacement lost.Fix
Compute
mantissa * 10^exponentin 128-bit integer arithmetic using__uint128_t, then convert to double with a single IEEE round-to-nearest-even cast. Supported for|exp| in [0, 19]where10^|exp|fits inuint64; cases outside that range (or otherwise outside the fast path's preconditions) still fall through tostrtod().Code delta:
src/fast_float_strtod.c(widened path — no new deps, pure C, no libstdc++ reintroduced)fastFloatTesttests/unit/type/zset.tclMeasured perf
1Mkeys-load-zset-listpack-with-100-elements-double-score, n=3 per branch per runner:Both arches recover the regression and then some. ARM goes from −27.4% (worst regression in the bisect) to +34.2% vs 8.6.
Correctness
Bit-exact with
strtod()on every tested case.Unit tests (
./redis-server test fastfloat): 98 passed, 0 failed. 21 new cases exercising:2^53boundary + ties-to-even rounding__uint128_tproduct±19and cases that fall through tostrtodTCL integration tests (
./runtest --single unit/type/zset):ZSCORE after a DEBUG RELOADtest exercises random-double round-trip and passes on bothlistpackandskiplistencodings.ZSCORE 17-19 significant digit mantissas (widened fast path)test explicitly asserts bit-exactZADD → DEBUG RELOAD → ZSCOREround-trip on a curated set of 17-19 digit scores that hit the new code path.References
The fast-path concept is Clinger's, the pure-C impl being extended is @antirez / @sundb / @mpozniak95 / @moticless's, the broader algorithmic context is
fast_float(Lemire & Magalhães). None of those directly cover the narrow "widen the Clinger fast path via__uint128_tone-shot" trick this patch uses — that was written for this patch.mantissa ≤ 2^53tomantissa ≤ 2^64.fast_float_strtod.cbeing extended: Replace fast_float C++ library with pure C implementation #14661 by @antirez, @sundb, @mpozniak95, @moticless. The existing Clinger fast path (mantissa ≤ 2^53) is unchanged; this patch adds a parallel branch for mantissa > 2^53.|exp| ≤ 19).The specific
__uint128_t-based "mantissa × 10^e" / "mantissa ÷ 10^|e|" with single-rounding conversion was written for this PR and is not a direct port from any cited work.Note
Medium Risk
Touches core numeric parsing logic used across Redis, so any rounding/edge-case mistake could affect persisted scores and comparisons; mitigated by tight bounds (
|exp|<=19) and added unit/integration tests.Overview
Improves decimal parsing performance by extending
fast_float_strtod.c’s fast path to handle mantissas larger than2^53(17–19 significant digits) using__uint128_tmultiply/divide for|exponent| <= 19, reducing fallbacks tostrtod()while keeping correct rounding.Adds targeted coverage for this widened path in the
fastFloatTesttable (boundary/ties-to-even, positive/negative exponents, negatives) and introduces a newZSCOREintegration test intests/unit/type/zset.tclthat validates bit-exactZADD/ZSCOREround-trips (including afterDEBUG RELOAD) for representative 17–19 digit scores.Reviewed by Cursor Bugbot for commit e1e7faf. Bugbot is set up for automated code reviews on this repo. Configure here.