Optimize result representation for BcMathCalculator #748

bcremer · 2023-04-06T11:45:14Z

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.

> vendor/bin/phpbench run --retry-threshold=3 --iterations=15 --revs=1000  --warmup=2 'benchmark/MoneyOperationBench.php' '--ref=before_bcmath_scale'
PHPBench (1.2.10) running benchmarks... #standwithukraine
with configuration file: /home/pushapidev/current/moneyphp/phpbench.json
with PHP version 8.1.17, xdebug ✔, opcache ✔
comparing [actual vs. before_bcmatch_scale]

\Benchmark\Money\MoneyOperationBench

    benchAdd................................R4 I9 - [Mo0.298μs vs. Mo0.610μs] -51.12% (±0.63%)
    benchSubtract...........................R1 I5 - [Mo0.298μs vs. Mo0.611μs] -51.34% (±1.04%)
    benchMultiply...........................R1 I12 - [Mo0.622μs vs. Mo0.629μs] -1.16% (±1.30%)
    benchDivide.............................R1 I13 - [Mo0.731μs vs. Mo0.726μs] +0.67% (±1.08%)
    benchSum................................R1 I12 - [Mo0.646μs vs. Mo1.063μs] -39.23% (±0.65%)
    benchMin................................R1 I14 - [Mo0.494μs vs. Mo0.493μs] +0.11% (±1.01%)
    benchMax................................R1 I6 - [Mo0.509μs vs. Mo0.512μs] -0.48% (±0.74%)
    benchAvg................................R1 I3 - [Mo1.462μs vs. Mo1.822μs] -19.79% (±1.40%)
    benchRatioOf............................R1 I0 - [Mo0.365μs vs. Mo0.349μs] +4.68% (±1.99%)
    benchMod................................R6 I13 - [Mo0.382μs vs. Mo0.383μs] -0.48% (±1.06%)
    benchIsSameCurrency.....................R1 I6 - [Mo0.059μs vs. Mo0.060μs] -1.88% (±1.31%)
    benchIsZero.............................R1 I11 - [Mo0.106μs vs. Mo0.105μs] +0.25% (±1.22%)
    benchAbsolute...........................R3 I14 - [Mo0.157μs vs. Mo0.157μs] -0.27% (±1.18%)
    benchNegative...........................R1 I9 - [Mo0.392μs vs. Mo0.710μs] -44.84% (±0.90%)
    benchIsPositive.........................R2 I6 - [Mo0.102μs vs. Mo0.102μs] -0.03% (±0.86%)
    benchCompare............................R3 I14 - [Mo0.133μs vs. Mo0.133μs] +0.06% (±0.98%)
    benchLessThan...........................R2 I3 - [Mo0.149μs vs. Mo0.149μs] -0.07% (±0.75%)
    benchLessThanOrEqual....................R2 I8 - [Mo0.149μs vs. Mo0.149μs] -0.01% (±1.25%)
    benchEquals.............................R1 I4 - [Mo0.169μs vs. Mo0.169μs] +0.13% (±0.93%)
    benchGreaterThan........................R1 I7 - [Mo0.149μs vs. Mo0.149μs] +0.11% (±1.60%)
    benchGreaterThanOrEqual.................R1 I14 - [Mo0.149μs vs. Mo0.149μs] +0.28% (±1.28%)

Subjects: 21, Assertions: 0, Failures: 0, Errors: 0

For full history: #747

bcremer · 2023-04-06T12:01:25Z

@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.

Ocramius · 2023-04-06T12:30:19Z

@bcremer changes seem to indicate that removing the scale broke everything in the test suite?

bcremer · 2023-04-06T12:32:26Z

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.

frederikbosch · 2023-04-06T12:37:08Z

@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.

Ocramius

I'm saying that your patch should NOT touch src/Calculator/BcMathCalculator.php unless there's a strong reason to do it.

The fact that tests cover that is because there's a BC boundary there.

tests/Calculator/BcMathCalculatorTest.php

frederikbosch · 2023-04-06T12:41:42Z

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.

bcremer · 2023-04-06T12:46:18Z

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%)

frederikbosch · 2023-04-06T12:52:41Z

I think this good to go, @Ocramius approved?

benchmark/MoneyOperationBench.php

Ocramius · 2023-04-06T12:57:07Z

src/Calculator/BcMathCalculator.php

@@ -31,13 +32,17 @@ public static function compare(string $a, string $b): int
    /** @psalm-pure */
    public static function add(string $amount, string $addend): string
    {
-        return bcadd($amount, $addend, self::SCALE);
+        $scale = str_contains($amount, '.') ? self::SCALE : 0;


This is checking $amount, but not $addend

I'd really just leave these methods alone :P

In fact, why not patch bcmath itself to ignore the scale directly, when there's no decimal stuff going on? :P

We could submit a PR for that too, agreed. Let's do both.

That would be a BC break in the bcmath extension wouldn't it?

It's not about ignoring the scale during the bcmath operation but about the return value of bcmath.

See previous PR for detailed explanation.

Not a breakage, if there's no behavioral change.

If there's noticeable performance overhead, a large chunk of the PHP community could benefit from it.

I need beauviour change for the performance improvement in the MoneyPHP ctor.

See #747 (comment)

Sorry, lemme rephrase: I was saying that patching https://github.com/php/php-src/ is not a BC break if there's no behavioral change.

I think we talk past each other.

bcadd('5', '15', 0) and bcadd('5', '15', 14) are already comparable performance wise.

What is different is the returned string value:

bcadd('5', '15', 0); // string(2) "20" bcadd('5', '15', 14); // string(2) "20.00000000000000"

I'd need to change in the php-core.

- bcadd('5', '15', 14); // string(2) "20.00000000000000" + bcadd('5', '15', 14); // string(2) "20"

I consider this a major BC break.

The actual performance improvement comes from passing the calculatores return value into Money::__construct

return new Money("20", $currenty); // fast return new Money("20.00000000000000", $currency); // slow

This is due to additional number parsing if the passed string is valid for FILTER_VALIDATE_INT :

money/src/Money.php

Line 80 in 8953b87

if (filter_var($amount, FILTER_VALIDATE_INT) === false) {

That given maybe it's would be a better fix to improve that validation for numeric strings containing int values.

So a fast coercion from 20.00000000000000 to 20.

I hope that clears things up. Sorry for splitting my PR into two, that whas not helpful to preserve context.

…lculator::substract

src/Calculator/BcMathCalculator.php

frederikbosch · 2023-04-11T09:10:38Z

Thanks @bcremer for your effort and PR for the optimization. @Ocramius thanks for your feedback!

* Optimize result representation for BcMathCalculator::add and BcMathCalculator::substract

Tunechi2323

Everything is good

bcremer mentioned this pull request Apr 6, 2023

Introduce PerformanceOptimizedDelegateCalculator #747

Closed

bcremer force-pushed the improve-BcMathCalculator-performance branch from c1ac815 to d5389be Compare April 6, 2023 11:46

bcremer added 2 commits April 6, 2023 13:47

Fix wrong method calls in MoneyOperationBench

2059332

Introduce calculator benchmarks

6a86252

bcremer force-pushed the improve-BcMathCalculator-performance branch from d5389be to 0bddd72 Compare April 6, 2023 11:48

Ocramius suggested changes Apr 6, 2023

View reviewed changes

tests/Calculator/BcMathCalculatorTest.php Outdated Show resolved Hide resolved

bcremer force-pushed the improve-BcMathCalculator-performance branch from 4e8ea5e to 2ad0ec7 Compare April 6, 2023 12:44

bcremer force-pushed the improve-BcMathCalculator-performance branch from 2ad0ec7 to c44e629 Compare April 6, 2023 12:48

Ocramius suggested changes Apr 6, 2023

View reviewed changes

Optimize result representation for BcMathCalculator::add and BcMathCa…

e3d208a

…lculator::substract

bcremer force-pushed the improve-BcMathCalculator-performance branch from c44e629 to e3d208a Compare April 6, 2023 13:00

frederikbosch reviewed Apr 6, 2023

View reviewed changes

src/Calculator/BcMathCalculator.php Outdated Show resolved Hide resolved

Improve decimal detection

714db87

frederikbosch merged commit 8162ab8 into moneyphp:master Apr 11, 2023

frederikbosch pushed a commit that referenced this pull request Nov 22, 2023

Optimize result representation for BcMathCalculator (#748)

6708c7a

* Optimize result representation for BcMathCalculator::add and BcMathCalculator::substract

Tunechi2323 reviewed Jan 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize result representation for BcMathCalculator #748

Optimize result representation for BcMathCalculator #748

bcremer commented Apr 6, 2023

bcremer commented Apr 6, 2023 •

edited

Loading

Ocramius commented Apr 6, 2023

bcremer commented Apr 6, 2023 •

edited

Loading

frederikbosch commented Apr 6, 2023

Ocramius left a comment

frederikbosch commented Apr 6, 2023

bcremer commented Apr 6, 2023 •

edited

Loading

frederikbosch commented Apr 6, 2023

Ocramius Apr 6, 2023

Ocramius Apr 6, 2023

Ocramius Apr 6, 2023

frederikbosch Apr 6, 2023

bcremer Apr 6, 2023

Ocramius Apr 6, 2023

bcremer Apr 6, 2023

Ocramius Apr 6, 2023

bcremer Apr 6, 2023 •

edited

Loading

frederikbosch commented Apr 11, 2023

Tunechi2323 left a comment

Optimize result representation for BcMathCalculator #748

Optimize result representation for BcMathCalculator #748

Conversation

bcremer commented Apr 6, 2023

bcremer commented Apr 6, 2023 • edited Loading

Ocramius commented Apr 6, 2023

bcremer commented Apr 6, 2023 • edited Loading

frederikbosch commented Apr 6, 2023

Ocramius left a comment

Choose a reason for hiding this comment

frederikbosch commented Apr 6, 2023

bcremer commented Apr 6, 2023 • edited Loading

frederikbosch commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcremer Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

frederikbosch commented Apr 11, 2023

Tunechi2323 left a comment

Choose a reason for hiding this comment

bcremer commented Apr 6, 2023 •

edited

Loading

bcremer commented Apr 6, 2023 •

edited

Loading

bcremer commented Apr 6, 2023 •

edited

Loading

bcremer Apr 6, 2023 •

edited

Loading