Optimize hot paths, and add a benchmark suite to keep them measurable - #68
Merged
Merged
Conversation
…tting The library's core operations were dominated by per-digit BigInteger division and by allocating temporary PreciseNumber instances for every comparison. This reworks those paths without changing the public API. Representation - Count significant digits from the significand's bit length plus a couple of comparisons instead of dividing by 10 once per digit, which was O(n^2) in the digit count. - Strip trailing zeros with a binary search on divisibility rather than one division per zero. - Cache the first 128 powers of ten and use them everywhere a power of ten is needed. Comparison - Route every comparison operator, Equals and CompareTo through a single Compare primitive that short circuits on sign and on decimal magnitude, and scales significands only when the two numbers share a decade. This makes comparison allocation-free; previously each one built two temporary instances, and CompareTo did the work twice. Arithmetic - Multiply no longer scales both operands to a common exponent first. a*10^x * b*10^y is (a*b)*10^(x+y), so commonizing only inflated both significands before multiplying them. - Add, Subtract, Divide and Mod scale raw significands instead of building intermediate instances. - Pow uses exponentiation by squaring instead of a linear multiply loop. - CreateRepeatingDigits uses the closed form digit * (10^n - 1) / 9. Text - TryFormat writes the sign, digits, padding zeros and decimal separator straight into the caller's span instead of composing a string first. It also no longer clears the whole destination buffer, and its length check is now correct for negative exponents; previously it could reject a buffer that was in fact large enough. - Parse collects digits and hands them to BigInteger.Parse once rather than doing a BigInteger multiply per character. - Float conversion formats into a stack buffer and parses spans instead of allocating several strings and arrays per conversion. Conversion - ToPreciseNumber caches each type's conversion strategy instead of calling GetInterfaces() on every call, and no longer boxes value types to inspect their runtime type. - As<T> caches the reflected copy constructor per target type. Two behavioural changes, both fixes: - Divide previously scaled the remainder and divisor by 10^exponent, a factor that cancels. For operands past the double range it overflowed to infinity or underflowed to zero and produced NaN, so Divide threw ArgumentOutOfRangeException for any pair beyond roughly 1e308. Dropping the factor fixes that and is measurably closer to the exact quotient. - Parse now throws FormatException for input with no digits, such as "-" or ".", instead of returning zero. Verified with a differential fuzz harness against the previous implementation: 866,510 checks over construction, digit counts, arithmetic, comparison, rounding, formatting, parsing and primitive conversion, with no divergence outside Divide's last double digit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018eTdSeGPQHGUKf9V3c2yXw
The optimization work in this branch was driven by an ad-hoc harness that lived outside the repository, so none of it was reproducible by anyone else. This makes the measurement part of the project. PreciseNumber.Benchmarks covers construction, comparison, arithmetic, integer powers, rounding, text conversion, and conversion to and from the primitive numeric types, one class per area. Most classes are parameterised by significant digit count: 8, 30 and 200. That axis is the point. Digits live in a BigInteger, so an operation that touches them one at a time looks fine at 8 digits and collapses at 200, which is exactly how the quadratic digit loops this branch removed went unnoticed. Comparison and arithmetic additionally separate operands whose exponents are far apart from operands in the same decade, because aligning exponents is its own cost. Allocation is reported alongside time in every table. Both matter here: every operation returns a new instance, so avoiding an intermediate shows up in Allocated before it shows up in Mean, and a comparison that allocates at all is a regression. Operands are derived from a fixed digit pattern rather than a random source so that two runs measure the same work, and the conversion inputs are held in fields rather than written as literals so the JIT cannot fold the conversion away. Notes on the wiring: - The library exposes its internals to the benchmark assembly so that the constructor can be measured directly rather than through Parse. - The benchmark assembly is named PreciseNumber.Benchmarks rather than taking ktsu.Sdk's automatic ktsu. prefix. BenchmarkDotNet locates the project that produced a benchmark assembly by matching the assembly name against .csproj file names, and with the prefix it finds nothing and reports NA for every benchmark. The project is never packaged, so the prefix buys nothing. - The project does not reference Polyfill, which KTSU0001 would otherwise require. It targets the current framework only, and the library already exposes its internal copy here, so a second reference would make every polyfilled member ambiguous. The Benchmarks workflow is dispatch-only and is deliberately not a gate. Shared runners vary by more than most changes worth catching, so a threshold there would either never fire or fire constantly. It takes a filter and a measurement length, writes the tables to the run summary, and archives the reports against the commit that produced them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018eTdSeGPQHGUKf9V3c2yXw
The new benchmark suite exposed a cliff at exactly 128 significant digits, the size of the fixed power of ten cache: 127 digits: 185 ns, 40 B 128 digits: 2,075 ns, 120 B 200 digits: 2,313 ns, 264 B Counting a value's digits compares it against a power of ten, and aligning two exponents multiplies by one. Neither is occasional: every construction does the first, so past the cache every operation on a wide value paid for a fresh BigInteger.Pow, and the cost jumped elevenfold over one digit rather than rising gradually. The cache now grows on demand, doubling up to 1024 entries and computing anything beyond that per call so one extreme exponent cannot leave a large cache behind. Growing replaces the array rather than filling the existing one: a BigInteger is a multi-field struct, so writing one into a shared array is not atomic and a concurrent reader could observe it half written, whereas publishing an already populated array through a single reference assignment cannot tear. Two threads growing at once build two correct arrays and one wins. At 200 significant digits, against the same benchmarks: construction, no trailing zeros 1,570 ns -> 257 ns (264 B -> 40 B) construction, unsanitized 1,396 ns -> 34 ns Negate 1,630 ns -> 256 ns (264 B -> 40 B) Add 1,845 ns -> 411 ns (488 B -> 264 B) Multiply 4,081 ns -> 1,032 ns (616 B -> 232 B) Squared 4,148 ns -> 894 ns (616 B -> 232 B) Values inside the original 128 entries are unaffected. Adds tests covering Pow10 across both boundaries, ascending and descending so that a cache grown past the request is exercised too, and confirms it still rejects a negative exponent. The differential fuzz harness against the previous implementation still reports 866,510 checks with no failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018eTdSeGPQHGUKf9V3c2yXw
SonarCloud's quality gate failed the previous commit on two conditions.
The blocker is mine and real: the Run Benchmarks step interpolated
${{ inputs.filter }} directly into its run block. That input is free-form
text supplied by whoever dispatches the workflow, so its contents were
expanded into the script body and would have run as shell. Both inputs now
reach the script through the environment, where no expansion happens.
The second condition was coverage on new code, which fell to 70.3% because
the benchmark project counted as uncovered new code. Benchmarks are
tooling rather than shipped behaviour, the same category as the test code
the analysis already exempts, so they join that exclusion list. Nothing
about the library's own coverage requirement changes.
Also extracts the parse failure message into a constant. It appeared
three times before this branch and four after, which Sonar flagged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018eTdSeGPQHGUKf9V3c2yXw
|
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
Four commits:
BigIntegerdivision (while (n != 0) { n /= 10; }), which is O(n²) in the digit count, and by allocating two temporaryPreciseNumberinstances for every comparison. No public API changes.PreciseNumber.Benchmarks, a BenchmarkDotNet suite, so this kind of regression is visible next time without an out-of-tree harness.Measured on an ad-hoc before/after harness (Release, net10.0) — the in-repo suite measures the current code only, so these numbers come from compiling both implementations side by side:
Pow(x, 20)==on 200-digit values*with wide exponent gap==(differing exponents)*CompareToRound(3)Parse(long decimal)ToStringParse207-digit significand+/ToPreciseNumber(double)ToPreciseNumber(int)All comparison operators are now allocation-free.
Optimizations
Representation
BigIntegercomparisons, instead of dividing by 10 once per digit.Comparison
EqualsandCompareTonow route through oneCompareprimitive that short circuits on sign and on decimal magnitude (exponent + significantDigits), scaling significands only when both numbers occupy the same decade. Previously each comparison built two temporary instances, andCompareTodid the whole thing twice.Arithmetic
Multiplyno longer scales both operands to a common exponent first —a·10ˣ · b·10ʸ = (a·b)·10ˣ⁺ʸ, so commonizing only inflated both significands before multiplying them.Add,Subtract,DivideandModscale raw significands instead of building intermediate instances.Powuses exponentiation by squaring instead of a linear multiply loop.CreateRepeatingDigitsuses the closed formdigit · (10ⁿ − 1) / 9.Text
TryFormatwrites the sign, digits, padding zeros and decimal separator straight into the caller's span instead of composing a string first.Parsecollects digits and callsBigInteger.Parseonce, rather than aBigIntegermultiply per character.Conversion
ToPreciseNumbercaches each type's conversion strategy instead of callingGetInterfaces()on every call, and no longer boxes value types to inspect their runtime type.As<T>caches the reflected copy constructor per target type.Behavioural changes
Three, all fixes:
Divideno longer throws for operands beyond thedoublerange. It scaled both the remainder and the divisor by10^commonExponent, a factor that cancels. Past ~1e308 that overflowed to infinity (or underflowed to zero), givingNaN, which the subsequent conversion rejected:Dropping the factor also makes the result closer to the exact rational quotient — over 1 000 random pairs, the new result was closer 162 times, the old one 56, identical 782.
TryFormat's length check is now correct for negative exponents.SignificantDigits + Exponent + 2underestimates when the exponent is negative, so it could accept a buffer too small to hold the output and then fail in the copy. It also no longer clears the caller's entire destination span — onlycharsWrittencharacters are touched.ParsethrowsFormatExceptionfor input with no digits ("-","."), instead of silently returning zero.Benchmark suite
PreciseNumber.Benchmarkscovers construction, comparison, arithmetic, integer powers, rounding, text conversion, and conversion to and from the primitive numeric types — one class per area.Most classes are parameterised by significant digit count: 8, 30 and 200. That axis is the point. Digits live in a
BigInteger, so an operation that touches them one at a time looks fine at 8 digits and collapses at 200, which is exactly how the quadratic loops removed here went unnoticed. Comparison and arithmetic additionally separate operands whose exponents are far apart from operands in the same decade, because aligning exponents is its own cost.Allocation is reported alongside time in every table. Both matter: every operation returns a new instance, so avoiding an intermediate shows up in
Allocatedbefore it shows up inMean, and a comparison that allocates at all is a regression.Three notes on the wiring, all commented in place:
Parse.PreciseNumber.Benchmarksrather than takingktsu.Sdk's automaticktsu.prefix. BenchmarkDotNet locates the project that produced a benchmark assembly by matching the assembly name against.csprojfile names; with the prefix it finds nothing and reportsNAfor every benchmark. The project is never packaged, so the prefix buys nothing.KTSU0001would otherwise require. It targets the current framework only, and the library already exposes its internal copy there, so a second reference would make every polyfilled member ambiguous.The
Benchmarksworkflow is dispatch-only and deliberately not a gate — shared runners vary by more than most changes worth catching, so a threshold there would either never fire or fire constantly. It takes a filter and a measurement length, writes the tables to the run summary, and archives the reports against the commit that produced them.What the suite found immediately
A hard cliff at exactly 128 significant digits — the size of the fixed power of ten cache introduced in commit 1:
Counting a value's digits compares it against a power of ten, and every construction does that, so past the cache every operation on a wide value paid for a fresh
BigInteger.Pow. The cache now grows on demand, doubling to 1024 entries and computing beyond that per call. Growing replaces the array rather than filling it in place: aBigIntegeris a multi-field struct, so writing one into a shared array is not atomic and a concurrent reader could observe it half written, whereas publishing an already populated array through a single reference assignment cannot tear.At 200 significant digits, against the committed benchmarks:
NegateAddMultiplySquaredValues inside the original 128 entries are unaffected.
Security fix
SonarCloud flagged a blocker in the benchmark workflow added by commit 2: the
Run Benchmarksstep interpolated${{ inputs.filter }}directly into itsrunblock. That input is free-form text supplied by whoever dispatches the workflow, so its contents were expanded into the script body and would have executed as shell. Both inputs now reach the script through the environment, where no expansion happens.The same analysis also failed on coverage of new code, because the benchmark project counted as uncovered. Benchmarks are tooling rather than shipped behaviour, the same category as the test code the analysis already exempts, so they joined that exclusion list. Nothing about the library's own coverage requirement changed — it reports 95.1% on this PR.
Testing
dotnet test— 220 passing (12 new regression tests covering digit-count boundaries,Pow10across both cache boundaries ascending and descending, long trailing-zero runs, wide exponent gaps, extreme-exponent division, large integer powers, exact-size and buffer-preservingTryFormat, and digit-less parse input).CompareTo,Round,ReduceSignificance,ToString,Parse,Pow,CreateRepeatingDigits,TryFormatat every buffer size, and conversions fromdouble/float/long/decimal/Half. Zero divergence outside the last digit ofDivide's double-computed fractional part, as described above. Re-run after the cache change with the same result.Known limitation, not addressed here
Dividecomputes its fractional part through adouble, so it caps at roughly 16 significant digits regardless of input precision. It is visible in the benchmarks as a suspiciously flat line — at 200 digitsDividemeasures faster thanAdd, because the result it builds is small no matter how large the operands were. That is a design limitation rather than a performance one, and fixing it would change results across the board, so it is left alone and documented in the benchmark README — worth raising separately.🤖 Generated with Claude Code
https://claude.ai/code/session_018eTdSeGPQHGUKf9V3c2yXw