Conversation
Two bugs blocked composer benchmark out of the box: - benchmarks/ wasn't in autoload-dev psr-4, so run_benchmarks.php failed with Class not found. - IntegerBenchmark::benchmarkInt8Arithmetic multiplied 50 * 30 = 1500, overflowing Int8 (-128..127) and throwing OverflowException. Fixed to 5 * 3. Also added two new benchmark cases used by the optimisation work that follows: IntArray::fromTrusted and Int8::of.
Two changes to typed array construction:
1. Replace array_all/array_find closure validation with a single
foreach loop in IntArray and the validateFloats/validateStrings/
validateBytes helpers on ArrayAbstraction. The closure invocation
per element was the dominant cost on construction of large arrays.
IntArray(1000): 903us -> ~300us (3x faster).
2. Add an optional bool \$trusted = false to the constructor and a
public static fromTrusted(array): self factory on IntArray,
FloatArray, StringArray and ByteSlice. Callers who already know
their input is well-typed (typed query results, output of
array_map('intval', ...), etc.) can skip the per-element check
entirely. IntArray::fromTrusted(1000): ~1.6us. Down from 6700x
slower than a native array assignment to roughly 12x.
The arithmetic hot path used to go through performOperation(callable, string) which invoked a [\$this, 'addValues'] array-callable per op. Four call frames + a re-validation in setValue() for each add(). Three changes: 1. Inline add/subtract/multiply/divide/mod directly in NativeArithmeticOperationsTrait and BigArithmeticOperationsTrait. One method, native math, two bounds comparisons, one allocation. 2. Add optional bool \$trusted = false to AbstractNativeInteger and AbstractBigInteger constructors. When true, skips the MIN/MAX check in setValue(). Used internally by arithmetic ops which already pre-validate the result before constructing. Removes a duplicate bccomp pair per Int64 op. 3. Remove the now-unreachable performOperation() and the five addValues/subtractValues/multiplyValues/divideValues/modValues helpers from both abstracts, plus the abstract performOperation declaration from both traits. ~190 lines of dead code. Public API unchanged. Same exceptions thrown (Overflow/Underflow/DivisionByZero/UnexpectedValueException). Numbers (100k iters): Int8 arithmetic: 11.67us -> 5.32us (2.2x) Int64 arithmetic: 14.56us -> 6.82us (2.1x)
The Int8 domain is exactly 256 values and instances are immutable, so they can be safely shared. Add a lazy static cache keyed by value. Int8::of(42) ~0.50us new Int8(42) ~1.20us 2.4x faster on cache hits. Memory bounded at ~8KB worst case (256 instances). Useful in tight loops; for one-off construction the allocator is already cheap enough. The cache is process-lifetime, which suits long-running PHP workers (Octane, Swoole, ReactPHP) and is effectively per-request under CGI. Not applied to wider integer types (Int16+) on purpose: 65k+ instances is too much for an unbounded cache. UInt8 and Byte could get the same treatment in a follow-up.
Arithmetic ops (add/subtract/multiply/divide) used to return new static(\$result) which re-ran setValue() — duplicate infinity + MIN/MAX checks on every op. Inline those checks once in the arithmetic methods, then construct via new static(\$result, true) to skip the second pass. Same pattern as AbstractNativeInteger/AbstractBigInteger. Float32 arithmetic now ~4.0us/op. Same exception thrown (OutOfRangeException) for the same conditions (INF, MIN/MAX overflow).
Mirrors Int8::of(). Both domains are exactly 256 values, instances
are immutable, so sharing is safe and bounded (~8KB worst case
per class).
new UInt8(42) ~0.94us
UInt8::of(42) ~0.51us (1.8x)
new Byte(42) ~0.49us
Byte::of(42) ~0.46us (marginal — Byte's constructor is
already a single comparison)
Perf/optimizations
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR refactors the type system to support trusted constructors that skip validation, implements flyweight caching for scalar types via ChangesType System Optimization with Trusted Construction and Arithmetic Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.55)Composer install failed: the lock file is not up to date with the latest changes in composer.json. Run Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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
1 participant
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 by CodeRabbit
New Features
of()factory methods to numeric types for fast, cached instance retrieval.first(),last(), andisEmpty()collection accessors.fromTrusted()factory methods for arrays to skip validation when creating pre-validated collections.Performance
Chores