Restore the simple loop in fastfloat_strncasecmp - #412
Merged
Merged
Conversation
PR #356 replaced the 3/5-character case-insensitive compare used by parse_infnan with SWAR helpers (fastfloat_strncasecmp3/5 plus a generic memcpy-based fastfloat_strncasecmp). That code only runs for "nan", "inf" and "infinity" inputs, so it cannot help ordinary parsing, but it grew the inlined inf/nan tail of the parser: on x86-64 (GCC 14 and clang 21) the per-commit benchmarks dropped by 1.5-2.5% because the extra dead code changed inlining decisions and register allocation on the hot path (+7 instructions/float on canada.txt). Go back to the plain loop, which the compiler unrolls for the constant lengths 3 and 5, and drop the now-unused generic SWAR function. The `actual < 256` guard of the original is unnecessary: OR-ing 0x20 into a value can only equal an ASCII lowercase letter when the value is that letter or its uppercase form, so the plain OR is exact for every character type.
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.
Summary
Reverts the SWAR case-insensitive compare introduced in #356 / #362 back to the plain 3/5-character loop, and removes the unused generic
fastfloat_strncasecmpSWAR variant (−125 lines).fastfloat_strncasecmpis only reached fromparse_infnan, i.e. fornan/inf/infinityinputs, so it cannot speed up ordinary number parsing. What #356 did instead was grow the inlined inf/nan tail of the parser, which perturbed the hot path.Investigation
The per-commit dashboard dropped by ~4% at #356 and #362 did not recover it (as expected: #362 only touched the
cpp20_and_in_constexpr()branch).Reproduced on an Intel Xeon (Gold 6548N),
realbenchmark, 5 interleaved rounds, medians, at the #356 merge commit vs its parent:The new code never executes on these inputs; the extra instructions/float come from codegen side effects:
parse_infnanwas a separate 0x17f-byte function before; after optimize fastfloat_strncasecmp #356 it gets inlined andfrom_chars_float_advanced<double,char>grows 2307 → 2891 bytes.parse_infnanwas already inlined; the function only grew by 10 instructions, but the register allocator reshuffled the live path and added spills inside the digit loop.On Apple M4 Max (Apple clang 17, hardware counters) the same change costs ±1 i/f — with 31 GPRs the added dead code creates no register pressure, which is why the PR author's M1 numbers looked flat.
Effect on current
mainMeasured against
mainat 6373592 (after #410), same machine, 5 interleaved rounds:#410 already moved the
!pns.validtail off the hot path, which absorbed the #356 regression on x86, so this PR is performance-neutral today (identical instruction counts with GCC, ±1–2 i/f with clang; clang'sparse_infnangoes back out of line). It remains a simplification and removes code that only ever cost performance in the benchmarks.Also tried: marking
parse_infnanfastfloat_never_inlineon top of this — clearly worse with GCC (+5–7 i/f), so not included.Tests
ctest15/15 (C++17),basictestwithFASTFLOAT_CONSTEXPR_TESTS=ON(C++20)static_asserts of the C++20 constexpr path fornan,NAN,-Inf,INFINITY, and the negative cases (nax,inx) — the branch fix early return error in fastfloat_strncasecmp #362 had to fix-fsyntax-only -Wall -Wextraunder C++11/14/20