Skip to content

Fix unbounded memory growth and hot path overhead - #19

Draft
bobmulder wants to merge 2 commits into
masterfrom
claude/memory-leaks-efficiency-3xjr56
Draft

Fix unbounded memory growth and hot path overhead#19
bobmulder wants to merge 2 commits into
masterfrom
claude/memory-leaks-efficiency-3xjr56

Conversation

@bobmulder

Copy link
Copy Markdown
Member

Analysis of the package for memory leaks and efficiency losses, plus the fixes for everything found.

The memory leak

AbstractNumber stored a strong reference to the instance it was derived from. Since every operation returns a new instance, a chain keeps its whole history alive: an accumulating loop grows without bound.

$total = new Number('0');
foreach ($rows as $row) {
    $total = $total->add($row); // every previous $total stays in memory
}

Measured with 50.000 additions: 7.596 KB retained, growing linearly (≈155 MB for a million rows). Discarding such a chain also destroyed it recursively, one stack frame per link.

The parent is now referenced through WeakReference, so an intermediate result is released as soon as nothing else refers to it. Same loop: 92 KB retained. parent() keeps working for every number that is still referenced, and returns null once the parent has been collected — documented in the README.

Efficiency losses

Before After
Fold of 50k additions (retained) 7.596 KB 92 KB
100k add('1.5') 0,069s 0,059s
100k isEqual() 0,052s 0,031s
100k isGreaterThanOrEqual() 0,048s 0,025s
100k isZero() 0,045s 0,007s
100k min()->max() 0,105s 0,047s
100k absolute() 0,009s 0,004s
20k Formatter::format() 0,302s 0,029s
20k Formatter::formatMoney() 0,587s 0,032s
100k round(2) 0,046s 0,145s
  • An instance per scalar operand. add('1.5') built a full Number (with its own parent link) just to read the string back out. Scalar operands are handled as strings now; getNumberFromInput() stays for implementations that use it.
  • A bcmath call per operand. Every operand was pushed through bcadd('0.0000', $value, 12). Padding with zeroes does not change a value, so that call is only made when an operand actually has more than 12 decimals.
  • Double comparisons. isGreaterThanOrEqual() and isLessThanOrEqual() ran two full comparisons (two operand conversions, two bccomp calls) where one bccomp answers both.
  • isZero() / isThirteen() went through isEqual(), which built an instance to compare against a constant.
  • absolute() / opposite() ran a bcmul by -1 to flip a sign character.
  • clamp() built three instances and converted its result to a string in between.
  • Formatter built a new NumberFormatter on every single call, while constructing one costs roughly twenty times as much as formatting a value with it. Formatters are cached per locale and options now (bounded to 32, Formatter::flush() empties it). Formatter::get() still hands out a fresh instance, so an instance you modify yourself is never shared.

Correctness fixed along the way

  • round(), ceil() and floor() cast to float, which defeats the purpose of a bcmath backed number. Beyond 15 significant digits the result was wrong (ceil('282913.00000000000002322266') returned 282913), and for large values (string) round(...) produced exponential notation which made every following bcmath call throw ValueErrorNumber::create('12345678901234567890.5')->round(0) was fatal. All three are calculated with bcmath now, including the four rounding modes (which had no test coverage). This is the one change that costs performance: exact rounding is ~1µs slower per call than a native float round.
  • Exponential notation is expanded on input. Number::create(0.00001) stored 1.0E-5, which no bcmath function accepts, so the value was unusable in any calculation.
  • min(), max(), clamp() and the divide() fallback truncated to 4 decimals, because they converted their value through __toString(). (new Number('1.123456789'))->min('2.987654321') returned 2.9876.
  • The divide() fallback accepted any type; it is validated like every other input now.

Verification

  • Existing suite green, plus 13 new tests (rounding modes, big number rounding, exponential input, precision of the limiting methods, formatter cache, parent lifetime, bounded memory).
  • Differential fuzzing of the old against the new implementation over 557.000 comparisons (arithmetic, comparisons, rounding, limiting, formatting, invalid input, values up to 20 decimals). Every difference found is one of the fixes listed above; there are no unintended behaviour changes.
  • php-cs-fixer clean, psalm reports the same single pre-existing error as master.

Found but not changed

jsonSerialize(): mixed uses PHP 8.0 syntax while composer.json requires ^7.4, so the package does not parse on PHP 7.4 and psalm has been failing on master since that change. Fixing it means either dropping 7.4 or dropping the return type — your call, so I left it alone.


Generated by Claude Code

claude and others added 2 commits August 15, 2026 13:51
Every derived instance held a strong reference to the instance it was
derived from, so a chain kept all of its intermediate results alive. An
accumulating loop grew without bound: folding 50k additions retained
7.6 MB. The parent is referenced weakly now, which brings that down to
92 KB while parent() keeps working for every number that is still
referenced.

Arithmetic and comparison methods no longer allocate an instance per
scalar operand, and no longer normalize operands that already fit the
internal scale. Comparisons run a single bccomp instead of building up
to two instances and comparing twice.

- round(), ceil() and floor() are calculated with bcmath instead of
  casting to float, which was lossy beyond 15 significant digits and
  produced exponential notation that made every following bcmath call
  throw a ValueError.
- Exponential notation is expanded on input, so floats like 0.00001 and
  strings like '1e3' can be used in calculations.
- min(), max(), clamp() and the divide() fallback no longer truncate
  their value to 4 decimals.
- Formatters are reused per locale and options instead of being rebuilt
  on every call, which makes formatting 10x (decimals) to 18x (money)
  faster.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oVaCdzr9zJS75QQcHKNzj

Copy link
Copy Markdown
Member Author

CI status

Green on PHP 8.0, 8.1 and 8.2. Two jobs are red, both for the same reason, and both are red on master as well:

Tests / P7.4 — 77 of the 78 tests pass, one errors:

1) MadeByBob\Number\Tests\NumberTest::testCanJsonSerialize
TypeError: Return value of MadeByBob\Number\AbstractNumber::jsonSerialize()
must be an instance of MadeByBob\Number\mixed, string returned
src/AbstractNumber.php:553

Psalm — one error, the same line:

ERROR: ReservedWord - src/AbstractNumber.php:551 - mixed is a reserved word

PHP 7.4 does not know mixed as a type, so it reads the return type as a class name. This came in with #17 and is untouched by this PR — I reproduced both failures locally against ab7a12e (the commit this branch starts from) with the same result. Every test added here passes on 7.4.

Fixing it is a support decision rather than a bug fix, which is why it is not in this PR:

  • Keep supporting 7.4: drop the return type and mark the method #[\ReturnTypeWillChange].
  • Or require PHP 8.0: raise php in composer.json and drop 7.4 from the two workflow matrices.

Happy to push either one — say which and I will add it here.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants