Investigate benchmark regressions in Max, CompareTo, ReduceSignificance, Pow, and Divide since 2.0 #76

Activity

1. matt-edmondson

   added

   enhancementNew feature or requestNew feature or request

   P4Backlog; nothing brokenBacklog; nothing broken

   on Sep 15, 2026
2. matt-edmondson commented on Sep 15, 2026

   

   matt-edmondson

   on Sep 15, 2026 – with Claude

   ContributorAuthor

   More actions

   Triage

   Category: Improvement (investigation)
   Priority: Low — consistent with the existing P4 label, which is already correct
   Area: PreciseNumber — Max, CompareTo, ReduceSignificance, Pow, Divide

   Low is the right call and the existing labels already say so: nothing is broken, correctness is unaffected, and the work is measurement before it is a fix. Worth keeping open rather than closing — a regression across five operations since 2.0 is a real signal, and performance debt that is never investigated is how a numerics library quietly stops being the fast option.

   Scoping note. This is an investigation issue, not an implementation one, and it is worth resisting the pull to fix anything before the measurement is in. Five named operations is broad enough that the regressions may well have different causes — or, equally plausibly, one shared cause in a common path that all five route through, in which case the fix is far smaller than the issue's surface suggests. Finding out which of those two it is should be the deliverable, and closing it may mean filing narrower follow-ups rather than landing a change here.

   Suggested assignment: unassigned is appropriate for now. This wants whoever knows the 2.0 change set, and it is a good candidate to pick up alongside other work in these code paths rather than scheduled on its own.

   Duplicates: none found in this repo or elsewhere in the org.
   Open PR covering this: none.

   Suggested next step: establish the regression is real and attributable before spending time on any one operation — confirm the benchmark numbers reproduce on current main, then bisect the 2.0 range for the five operations together and see whether they share a commit. If they do, this is one fix; if they don't, split it.

   ---

   Generated by Claude Code
3. matt-edmondson

   self-assigned this

   on Sep 22, 2026
4. matt-edmondson commented on Sep 22, 2026

   

   matt-edmondson

   on Sep 22, 2026 – with Claude

   ContributorAuthor

   More actions

   Measured. It is one cause, not five — and one of the five moved the other way.

   The triage asked the question that decides this issue's shape:

   bisect the 2.0 range for the five operations together and see whether they share a commit. If they do, this is one fix; if they don't, split it.

   Three of the five do not reproduce. One improved substantially. The two that remain share one cause, and it is the one the issue guessed.

   #95 is open with the baseline ref input that step 1 asks for. It does not close this.

   Method

   v1.9.0 against current main, both measured on the same machine in one session, sequentially — never concurrently, since two benchmark processes on one box measure contention rather than code.

   v1.9.0 is cd9a822, the commit this issue names as 1.x, so the baseline is the intended one and is reachable as a stable tag rather than a loose SHA.

   --job short, identical filters both sides. Noise below is the larger of the two BenchmarkDotNet error bars, as a percentage of the baseline mean; a row is only called reproduced where the change clears it.

   Results

   Benchmark	v1.9.0	main	change	noise
   Max (30)	2.87 ns	3.93 ns	+37.0%	15.5%	reproduced
   Max (200)	2.79 ns	3.81 ns	+36.6%	29.1%	reproduced
   Max (8)	3.03 ns	3.69 ns	+21.7%	23.2%	under noise
   CompareTo (200)	2.02 ns	2.73 ns	+35.5%	9.0%	reproduced
   CompareTo (30)	1.74 ns	2.05 ns	+18.2%	14.4%	reproduced
   CompareTo (8)	1.94 ns	2.01 ns	+3.5%	205%	under noise
   ReduceSignificance (200)	381.55 ns	159.38 ns	−58.2%	18.8%	improved
   ReduceSignificance (30)	187.06 ns	117.77 ns	−37.0%	100%	under noise
   ReduceSignificance (8)	65.53 ns	51.78 ns	−21.0%	36.0%	under noise
   Pow (2)	56.54 ns	64.25 ns	+13.6%	15.7%	under noise
   Pow (10)	349.17 ns	348.94 ns	−0.1%	14.9%	under noise
   Pow (64)	1512.24 ns	1507.43 ns	−0.3%	20.2%	under noise
   TryFormat (8)	41.27 ns	39.74 ns	−3.7%	12.7%	under noise
   TryFormat (30)	100.46 ns	94.17 ns	−6.3%	40.0%	under noise
   TryFormat (200)	737.08 ns	747.08 ns	+1.4%	24.8%	under noise
   Divide (8)	248.60 ns	251.00 ns	+1.0%	14.8%	under noise
   Divide (30)	390.30 ns	411.70 ns	+5.5%	7.7%	under noise
   Divide (200)	1747.30 ns	1768.50 ns	+1.2%	6.4%	under noise

   ReduceSignificance is now much faster, exactly as the issue's caveat predicted

   The issue flagged it at +19% and then said so itself:

   #72 rewrote the rounding in ReduceSignificance and Round, so that row may have moved in either direction.

   It moved, decisively: −58% at 200 digits, and faster at all three sizes. The row in the issue's table was measured before #72 and is stale. Nothing to investigate.

   The root cause of the two that remain

   Max's source is byte-for-byte identical on both refs:

   public static PreciseNumber Max(PreciseNumber x, PreciseNumber y) => x > y ? x : y;

   What changed is what that expression costs, because the type under it changed:

   // v1.9.0
   public record PreciseNumber            // reference type -- returns an 8-byte pointer
   
   // main
   public readonly partial record struct PreciseNumber   // value type -- returns a 24-byte copy

   sizeof(PreciseNumber) measured at 24 bytes on main, matching the issue's figure. So this is confirmation of the issue's own hypothesis rather than a new finding — Max "now returns a copy of a 24-byte struct where 1.x returned a reference" is precisely what the numbers say.

   CompareTo is the same conversion seen from the argument side: this is now a 24-byte by-value pass into Compare rather than a pointer. Its signature also lost the null check it no longer needs (CompareTo(PreciseNumber? other) → CompareTo(PreciseNumber other)), so the extra cost is the copy, not added work.

   Max's absolute cost is about +1 ns and does not scale with digits (+1.02, +1.06, +0.66 ns at 200/30/8). A cost flat across the Digits axis is not in the BigInteger work — it is fixed per-call overhead, which is what a struct copy looks like. CompareTo's delta is smaller and does grow with digits (+0.72/+0.32/+0.07), so its signature is less clean, but it reproduces at 30 and 200.

   Allocation is zero on both refs for both benchmarks, so nothing here is a hidden allocation.

   So: this is not five regressions

   It is one — the #70 value-type conversion — surfacing in the two benchmarks cheap enough for a 24-byte copy to be a visible fraction of them. Pow, TryFormat and Divide all do enough BigInteger work that the same copy disappears into the noise, which is why they showed small positive numbers in a single shared-runner run and show nothing here.

   That conversion is also the thing that made 2.0 worth doing: the abstraction-cost ratio in the benchmarks README went from 99.7×/293.9× at 1.9.0 to 82.8×/248.8× at 2.0.0. Paying ~1 ns on Max is the same change that took ~15% off arbitrary precision everywhere else. Whether that trade is worth unwinding at two call sites is a judgement, not a defect.

   Caveats, stated plainly

   • This ran in a container, not a GitHub-hosted runner. The same-machine property holds, which is the part that matters, but absolute magnitudes are not comparable with the issue's original table — it had Max (200) at +84% where I measure +37%.
   • --job short. The "under noise" rows are not disproven, only unresolved at this job length; Pow (2) at +13.6% against 15.7% noise is the one I would want a default-length run on before calling it nothing.
   • Three iterations on the two sub-4ns benchmarks is close to the floor of what BenchmarkDotNet can separate. CompareTo (8) came back with an error bar twice its mean.

   Suggested next step

   I have deliberately not touched Max or CompareTo. The obvious candidates — in parameters on Compare, AggressiveInlining on both — are exactly the kind of change that can make a readonly struct slower through defensive copies, and guessing at a 1 ns fix without measuring it is how this issue gets reopened.

   My read is that this issue has now answered what it asked and could close, with the Max/CompareTo copy cost split into a narrow follow-up if it is worth pursuing at all. Three of its five named operations are clean, its ReduceSignificance row is stale in the good direction, and what is left is one known, deliberate, well-compensated trade rather than a regression to hunt.

   Happy to run the comparison at --job default on the runner via #95 once that merges, if the Pow (2) row is worth settling.

   ---

   Generated by Claude Code
5. matt-edmondson

   removed their assignment

   on Sep 22, 2026
6. matt-edmondson commented on Sep 24, 2026

   

   matt-edmondson

   on Sep 24, 2026 – with Claude

   ContributorAuthor

   More actions

   Two of these rows are noise, and the code says why before any measurement does

   Working the "rerun the comparison" step. Three things came out, and the first is checkable from the source without a benchmark at all.

   Max cannot scale with digits for the operands it is given

   Max is x > y ? x : y, and Compare short circuits on sign and then on decimal magnitude, reaching CommonizeSignificands only when both operands occupy the same decade. ComparisonBenchmarks.Setup gives it:

   left  = Operands.Number(Digits, -10);
   right = Operands.Number(Digits, -40, offset: 7);

   Decimal magnitude is exponent + SignificantDigits, so those are Digits - 10 and Digits - 40 — thirty apart at every digit count. The magnitude short circuit fires at 8, 30 and 200 alike, and the significands are never touched. The returned struct is 24 bytes whatever the digits, because the BigInteger inside it is a reference plus a sign. So Max does identical O(1) work in all three rows, and the +23% / +17% / +84% shape in the table above is measuring the runner, not the type.

   Measured here (--job short, 21 benchmarks, one process), with each cell's own standard deviation:

   benchmark	8	30	200	span
   GetHashCode	1.33 ±0.02	13.90 ±0.21	26.14 ±0.49	1873%
   EqualsSameExponent	3.08 ±0.08	4.92 ±0.30	5.50 ±0.11	79%
   EqualsDifferentExponent	3.52 ±0.25	2.28 ±0.14	2.38 ±0.39	54%
   EqualsDifferentDecade	3.36 ±0.11	2.22 ±0.27	2.89 ±0.18	52%
   LessThan	2.17 ±0.10	2.09 ±0.03	2.26 ±0.00	8%
   CompareTo	2.16 ±0.05	2.25 ±0.19	2.08 ±0.16	8%
   Max	4.02 ±0.28	3.77 ±0.15	3.90 ±0.27	7%

   The suite separates exactly along the line the code draws. GetHashCode reads every digit and costs 20x more at 200. EqualsSameExponent is the one comparison whose operands share a decimal magnitude, so it falls through to BigInteger.Compare and nearly doubles. Everything that short circuits is flat, and the two ~50% rows are non-monotone (3.36 → 2.22 → 2.89), which is the signature of noise rather than of work.

   So the benchmark measures real digit-dependence where real digit-dependence exists. Max is not one of those places.

   The residual Max cost is real, and it is the trade #70 deliberately made

   Max sits ~1.7 ns above CompareTo in every row, flat in digits — which is what a fixed 24-byte copy plus a branch should look like, and matches the hypothesis in the issue. That part is not noise and is not fixable without undoing the value type: a method returning PreciseNumber by value cannot also return a reference. Every comparison benchmark here allocates 0 B, which is what was bought.

   CompareTo moved the other way in this repository's own history

   docs/benchmarks/history.json tracks ComparisonBenchmarks.CompareTo across eight releases, and every entry carries the same baselineNs of 454.8648 and runId: local-seed — one job, one machine, which is the comparability the issue asks for.

   1.9.0 (cd9a822) is public record PreciseNumber; 2.0.0 (da3aac6) is public readonly partial record struct — so that step is the value-type conversion:

   Digits	1.9.0	2.0.0	change
   8	4.6033	3.9819	−13.5%
   30	4.6786	4.1473	−11.4%
   200	4.7639	3.5786	−24.9%

   Faster, not slower, against the +16% / +16% / +5% in the table above.

   And the noise floor is measurable from the same file. Only two commits touched PreciseNumber.cs between 2.0.0 and 2.0.5 (3562ce1 formatting-length arithmetic, f97cc70 conversion edge cases) and neither touches Compare, CommonizeSignificands or any comparison operator — so those six entries run identical comparison code. Their spread:

   Digits	min	max	stdev	range
   8	3.5734	4.1605	0.2498	15.6% of mean
   30	3.5677	4.4563	0.3306	22.2% of mean
   200	3.5699	4.2016	0.2738	16.3% of mean

   Identical code, one machine, one job — and still 16–22%. The +16% under investigation is inside that.

   Caveats, and the one step already done

   • The table above is a short job on a shared cloud VM, so its absolute nanoseconds are not comparable to the issue's default-job GitHub-runner figures. What it establishes is the shape across the Digits axis within one run, which is the claim being tested. The history figures are likewise only compared within their own job.
   • The baseline ref input that suggested-next-step 1 asks for already exists in benchmarks.yml, with a comment citing this issue, measuring the baseline ref before the checkout on the same runner. So the tooling for a proper A/B is in place and just needs dispatching.

   Suggested disposition

   • Max digit-scaling, CompareTo (all three rows), TryFormat, Divide (+2–6%): below the noise floor established above — close unless a dispatched baseline run reproduces them.
   • Max absolute (~+1.7 ns over CompareTo, flat): real, understood, and the accepted cost of zero allocation. Worth writing into the docs rather than chasing.
   • ReduceSignificance (+19%) and Pow (+13%): not addressed here, and [patch] Fix conversion edge cases found in review #72 rewrote the rounding after the "2.0" reading was taken, so those two genuinely need the re-run.

   ---

   Generated by Claude Code
7. matt-edmondson commented on Sep 24, 2026

   

   matt-edmondson

   on Sep 24, 2026 – with Claude

   ContributorAuthor

   More actions

   Triage

   Category: Bug (performance investigation) — mostly disproved, scope now narrow
   Priority: Low (the existing P4 label is right)
   Area: ComparisonBenchmarks, ReduceSignificance, Pow

   Re-triaging because the measurement above (2026-09-24) removes most of this issue's subject matter, and the issue is open and unassigned.

   The strongest result here is the one that needed no benchmark at all: Max cannot scale with digit count for the operands the benchmark gives it. Compare short-circuits on decimal magnitude, ComparisonBenchmarks.Setup hands it operands thirty decades apart at every digit count, so the significands are never touched and the work is O(1) in all three rows. The reported +23% / +17% / +84% was measuring the runner. Establishing that from the code first, then confirming it with a run, is the right order and makes the conclusion much harder to argue with.

   The noise floor derivation is the other load-bearing piece: six history entries running identical comparison code (only 3562ce1 and f97cc70 touched PreciseNumber.cs between 2.0.0 and 2.0.5, neither touching Compare, CommonizeSignificands or any operator) still spread 16–22% on one machine, one job. Every row under investigation except two sits inside that. And CompareTo moved the other way across the actual value-type conversion (1.9.0 → 2.0.0: −13.5% / −11.4% / −24.9%), against the +16% / +16% / +5% this issue was filed on.

   Low because what is left is a measurement question with no user-visible defect behind it, and the type is faster than the issue claims on the row it leads with.

   Suggested assignment: none specific.

   Duplicates: none. #70 is the value-type conversion whose trade the residual Max cost (~+1.7 ns over CompareTo, flat in digits — a fixed 24-byte copy plus a branch) is the accepted price of; #72 rewrote the rounding after the "2.0" reading, which is why ReduceSignificance genuinely needs re-measuring.

   Already in progress / open PR: none covering this issue. Two open PRs touch code it mentions and are worth knowing about when the re-run happens, because either would shift a baseline: #103 (hash the sanitized form, so Equals and GetHashCode agree) lands on GetHashCode, the one benchmark with real 20x digit-scaling; #102 (reduce by the GCD before testing whether a quotient terminates) lands on the Divide path. Neither is a fix for anything in this issue — but re-running the suite before they merge and attributing the delta afterwards would confuse two effects.

   Suggested next step: dispatch the baseline ref input that already exists in benchmarks.yml — the tooling for a proper A/B is in place and cited to this issue, so this costs a workflow run, not development. Then re-scope this issue to ReduceSignificance (+19%) and Pow (+13%) only, and close out the rest: Max digit-scaling, CompareTo on all three rows, TryFormat and Divide are inside the established noise floor and should not be chased further without a reproduction that clears it.

   The Max absolute cost is worth a sentence in the docs rather than an investigation — it is understood, it is the zero-allocation trade, and a method returning PreciseNumber by value cannot also return a reference. Recording that will stop it being re-discovered as a regression.

   ---

   Generated by Claude Code
8. matt-edmondson commented on Sep 28, 2026

   

   matt-edmondson

   on Sep 28, 2026 – with Claude

   ContributorAuthor

   More actions

   Decision (maintainer, 2026-09-28)

   • The recommendation is accepted, and this issue is rescoped to ReduceSignificance and Pow only. Max digit-scaling, CompareTo (all rows), TryFormat and Divide are inside the established noise floor, so they are out of scope unless a dispatched baseline run reproduces them.
   • Dispatch the baseline A/B run with the existing baseline ref input in benchmarks.yml, v1.9.0 against main on one runner, to settle the two remaining rows.
   • The Max absolute cost (about +1.7 ns over CompareTo, flat in digits) is the accepted zero-allocation trade from [major] Make PreciseNumber a value type #70. Record it in the benchmark docs rather than investigating it.

   Next reader: dispatch benchmarks.yml with baseline set, ideally before or clearly separated from #102/#103 so their effects are not conflated. Then report the ReduceSignificance and Pow rows here, and add the docs note on Max. Close the issue if neither row clears the noise floor.

   ---

   Generated by Claude Code