Closed bcremer closed 1 year ago
@Ocramius You implemented
Tests\Money\Calculator\LocaleAwareBcMathCalculatorTest::itUsesScaleForSubtract
\Tests\Money\Calculator\BcMathCalculatorTest::itUsesScaleForSubtract
Both are currently failing with my changes.
From my understanding \Money\Calculator\BcMathCalculator::add
should only be called with int
like strings and never with float like strings.
Also the \Tests\Money\Calculator\CalculatorTestCase
only uses positive-int
for inputs.
@bcremer changes seem to indicate that removing the scale broke everything in the test suite?
It's only the explicit itUsesScale*
tests that fail. All other real-world tests are running fine.
The test was introduced by @frederikbosch in https://github.com/moneyphp/money/pull/528/files but the implementation changed quite a bit some time ago.
@bcremer I cannot recall or see why I would have done that. Maybe it was some preliminary work for PreciseMoney, a project that never made the finish line. If no other fails, I think it is fine to change the scale to zero for add and subtract operations.
Hmm, maybe @Ocramius is right. In the end of the day this is a performance patch. It should deal with all features in the library.
I see your point. I pushed a solution that will speedup the "real world path" as well as maintain the public api.
This will still result in a 50% improvement for certain operations:
comparing [actual vs. before_bcmath_scale]
\Benchmark\Money\MoneyOperationBench
benchAdd................................R5 I14 - [Mo0.324μs vs. Mo0.646μs] -49.89% (±0.52%)
benchSubtract...........................R1 I4 - [Mo0.323μs vs. Mo0.648μs] -50.24% (±0.60%)
benchMultiply...........................R1 I12 - [Mo0.670μs vs. Mo0.670μs] -0.00% (±1.14%)
benchDivide.............................R1 I7 - [Mo0.775μs vs. Mo0.780μs] -0.65% (±0.98%)
benchSum................................R1 I9 - [Mo0.713μs vs. Mo1.072μs] -33.54% (±0.98%)
benchMin................................R1 I6 - [Mo0.513μs vs. Mo0.518μs] -0.88% (±1.29%)
benchMax................................R1 I3 - [Mo0.527μs vs. Mo0.530μs] -0.60% (±1.10%)
benchAvg................................R1 I0 - [Mo1.596μs vs. Mo1.936μs] -17.59% (±1.10%)
benchRatioOf............................R1 I4 - [Mo0.359μs vs. Mo0.358μs] +0.45% (±0.47%)
benchMod................................R4 I13 - [Mo0.396μs vs. Mo0.394μs] +0.50% (±1.15%)
benchIsSameCurrency.....................R1 I6 - [Mo0.061μs vs. Mo0.061μs] +0.01% (±0.97%)
benchIsZero.............................R1 I3 - [Mo0.108μs vs. Mo0.109μs] -0.72% (±1.07%)
benchAbsolute...........................R4 I11 - [Mo0.161μs vs. Mo0.163μs] -1.09% (±0.86%)
benchNegative...........................R2 I10 - [Mo0.422μs vs. Mo0.755μs] -44.15% (±0.81%)
benchIsPositive.........................R1 I13 - [Mo0.108μs vs. Mo0.107μs] +0.98% (±1.05%)
benchCompare............................R1 I13 - [Mo0.134μs vs. Mo0.135μs] -1.05% (±1.27%)
benchLessThan...........................R5 I11 - [Mo0.154μs vs. Mo0.154μs] +0.02% (±1.07%)
benchLessThanOrEqual....................R2 I5 - [Mo0.154μs vs. Mo0.154μs] +0.34% (±0.99%)
benchEquals.............................R1 I8 - [Mo0.174μs vs. Mo0.174μs] -0.04% (±1.02%)
benchGreaterThan........................R5 I13 - [Mo0.154μs vs. Mo0.154μs] +0.07% (±1.07%)
benchGreaterThanOrEqual.................R1 I12 - [Mo0.154μs vs. Mo0.155μs] -0.88% (±0.70%)
I think this good to go, @Ocramius approved?
Thanks @bcremer for your effort and PR for the optimization. @Ocramius thanks for your feedback!
Optimize the result representation of some
BcMathCalculator
operations for faster usage in\Money\Money::__construct
.This will provide a 50% performance boost for some operations.
For full history: https://github.com/moneyphp/money/pull/747