Quick money (butchering this library for the glory of satan, and CPU cycles)

[Ocramius]

This is a draft: still playing around with it.

Ultimately, I'm mostly toying around with the library, removing bits that can be removed without substantial BC Breaks (yes, there will be BC breaks) and inlining code that can be inlined (yes, the code will be inlined).

I suggest avoiding an immediate code review until this is ready and I have stable performance results (most of the benchmarks were performed while my PC was under high load, so the values in the commit messages are not reliable).

In addition to all this, I still need to check if the GMP calculator can be used instead of BcMath, in order of priority, since GMP seems to generally be a bit quicker.

Notable improvements:

• full type coverage (100% type coverage by vimeo/psalm)
• 80%+ mutation test coverage (roave/infection-static-analysis-plugin)
• removal of a lot of internal API
• roughly 50% performance improvements in happy-path usage of Money, Number and Currency
• removal of Calculator discovery by-default (you want a custom calculator? configure it manually!)

Benchmark results

❯ php -dopcache.enable_cli=1 ./vendor/bin/phpbench run --ref=ref_original_5 --retry-threshold=3 --iterations=15 --revs=1000 --warmup=2
PHPBench @git_tag@ running benchmarks...
with configuration file: /home/ocramius/Documents/moneyphp/money/phpbench.json
with PHP version 8.0.3, xdebug ❌, opcache ✔

\Benchmark\Money\NumberInstantiationBench

    benchConstructorWithZeroIntegerAmount...R1 I2 - [Mo0.210μs vs Mo0.163μs] +28.42% (±1.29%)
    benchConstructorWithPositiveIntegerAmou.R1 I8 - [Mo0.308μs vs Mo0.626μs] -50.87% (±0.82%)
    benchConstructorWithNegativeIntegerAmou.R2 I2 - [Mo0.305μs vs Mo0.683μs] -55.40% (±0.90%)
    benchConstructorWithZeroAndFractionalAm.R1 I7 - [Mo0.316μs vs Mo0.576μs] -45.04% (±0.78%)
    benchConstructorWithFractionalAmount....R1 I14 - [Mo0.436μs vs Mo1.144μs] -61.85% (±0.80%)
    benchConstructorWithNegativeFractionalA.R1 I7 - [Mo0.435μs vs Mo1.200μs] -63.75% (±1.15%)
    benchFromStringWithZeroIntegerAmount....R1 I3 - [Mo0.319μs vs Mo0.208μs] +53.54% (±0.99%)
    benchFromStringWithPositiveIntegerAmoun.R1 I4 - [Mo0.426μs vs Mo0.676μs] -37.02% (±1.05%)
    benchFromStringWithNegativeIntegerAmoun.R1 I14 - [Mo0.423μs vs Mo0.731μs] -42.22% (±0.91%)
    benchFromStringWithZeroAndFractionalAmo.R2 I8 - [Mo0.476μs vs Mo0.689μs] -30.97% (±1.00%)
    benchFromStringWithFractionalAmount.....R2 I14 - [Mo0.609μs vs Mo1.257μs] -51.53% (±0.79%)
    benchFromStringWithNegativeFractionalAm.R1 I13 - [Mo0.612μs vs Mo1.304μs] -53.11% (±1.06%)

\Benchmark\Money\MoneyOperationBench

    benchAdd................................R1 I14 - [Mo1.244μs vs Mo1.338μs] -7.07% (±1.45%)
    benchSubtract...........................R1 I14 - [Mo1.254μs vs Mo1.315μs] -4.65% (±1.36%)
    benchMultiply...........................R3 I10 - [Mo1.297μs vs Mo2.346μs] -44.70% (±0.96%)
    benchDivide.............................R3 I14 - [Mo2.105μs vs Mo3.074μs] -31.52% (±1.03%)
    benchSum................................R1 I2 - [Mo2.231μs vs Mo3.630μs] -38.56% (±0.89%)
    benchMin................................R1 I14 - [Mo1.195μs vs Mo1.317μs] -9.22% (±1.13%)
    benchMax................................R1 I2 - [Mo1.191μs vs Mo1.307μs] -8.83% (±0.59%)
    benchAvg................................R2 I12 - [Mo1.190μs vs Mo1.309μs] -9.12% (±1.10%)
    benchRatioOf............................R2 I14 - [Mo0.783μs vs Mo1.232μs] -36.48% (±0.69%)
    benchMod................................R1 I4 - [Mo0.761μs vs Mo0.781μs] -2.60% (±1.24%)
    benchIsSameCurrency.....................R1 I14 - [Mo0.081μs vs Mo0.107μs] -24.24% (±0.96%)
    benchIsZero.............................R1 I1 - [Mo0.242μs vs Mo0.226μs] +7.02% (±0.96%)
    benchAbsolute...........................R1 I14 - [Mo0.345μs vs Mo0.307μs] +12.60% (±0.99%)
    benchNegative...........................R1 I14 - [Mo1.396μs vs Mo1.595μs] -12.48% (±1.01%)
    benchIsPositive.........................R1 I8 - [Mo0.236μs vs Mo0.219μs] +7.94% (±0.80%)
    benchCompare............................R1 I14 - [Mo0.320μs vs Mo0.361μs] -11.35% (±0.84%)
    benchLessThan...........................R1 I0 - [Mo0.351μs vs Mo0.394μs] -10.89% (±0.46%)
    benchLessThanOrEqual....................R1 I14 - [Mo0.354μs vs Mo0.386μs] -8.06% (±0.73%)
    benchEquals.............................R1 I14 - [Mo0.391μs vs Mo0.141μs] +177.89% (±0.96%)
    benchGreaterThan........................R1 I2 - [Mo0.352μs vs Mo0.386μs] -8.93% (±0.68%)
    benchGreaterThanOrEqual.................R1 I8 - [Mo0.352μs vs Mo0.387μs] -9.02% (±0.63%)

\Benchmark\Money\MoneyInstantiationBench

    benchConstructorWithZeroIntegerAmount...R2 I14 - [Mo0.149μs vs Mo0.175μs] -15.11% (±0.73%)
    benchConstructorWithPositiveIntegerAmou.R1 I0 - [Mo0.174μs vs Mo0.245μs] -29.24% (±1.32%)
    benchConstructorWithNegativeIntegerAmou.R1 I14 - [Mo0.175μs vs Mo0.248μs] -29.54% (±0.77%)
    benchConstructorWithZeroStringAmount....R1 I4 - [Mo0.183μs vs Mo0.162μs] +13.28% (±0.59%)
    benchConstructorWithPositiveStringAmoun.R1 I3 - [Mo0.205μs vs Mo0.180μs] +13.46% (±0.54%)
    benchConstructorWithNegativeStringAmoun.R1 I8 - [Mo0.209μs vs Mo0.185μs] +12.83% (±0.74%)

Subjects: 39, Assertions: 0, Failures: 0, Errors: 0

CI check status:

• Composer Normalize (pull_request) - Failing after 19s - somehow didn't check my updated composer.json? 🤔
• PrettyCI — Code formatting - no longer relevant - can be disabled?
• Build (5.6) Expected - no longer expected - need adjustment of branch protection rules pre-merge
• Build (7.0) Expected - no longer expected - need adjustment of branch protection rules pre-merge
• Build (7.1) Expected - no longer expected - need adjustment of branch protection rules pre-merge
• Build (7.2) Expected - no longer expected - need adjustment of branch protection rules pre-merge
• Build (7.3) Expected - no longer expected - need adjustment of branch protection rules pre-merge
• Build (7.4) Expected - no longer expected - need adjustment of branch protection rules pre-merge
• PHP-CS-Fixer Expected - no longer expected - need adjustment of branch protection rules pre-merge
• PHPStan Expected - no longer expected - need adjustment of branch protection rules pre-merge

Fixes

• fixes moneyphp/money declares compatibility with PHP >5.6, but cannot guarantee compatibility with PHP 9 or later #633 (PHP 8 compat is now declared with a stricter range)
• fixes PHP 8 compatibility #619 (PHP 8 compat)
• fixes Allow null amount #615 (no longer allowing null amount)
• fixes Update cached currencies #583 (interface of taggable cache in 1.0.0)
• fixes Wrong amount set causes unexpected comparison behaviour #575 ("5000\n" is NOT a numeric-string, so that value is now considered illegal at type level)
• fixes Construct Money from JSON stdClass #474 (interface is now much stricter: json de-serialization is completely up to the end-user)
• fixes Make Number and/or Calculators internal #465 (Calculator and Number are now internal)
• fixes Unspecified behavior of Money::allocate() #258 (negative ratio in Money#allocate() is unsupported)
• fixes Deal with decimals using PreciseMoney #7 (internals of Money now use numeric-string and BcMathCalculator by default)

[frederikbosch]

@sagikazarmark In my opinion these changes should target MoneyPHP 4.0 with PHP8.0+. This would mean that @Ocramius has the freedom to remove deprecation warnings and add types where possible. So then we have Money 3 for <= PHP7.4. I would then also create a separate branch for the development of 4.0. Would you agree?

[Ocramius]

@frederikbosch your maintenance role, your rules: remember that bumping dependencies is not a BC break, but this branch DOES contain BC breaks, so it is a good idea to release a new major anyway.

Can I suggest to branch off 3.x for the old stuff meanwhile (to allow for bugfixes to land)?

[frederikbosch]

@Ocramius That's why I asked the other maintainer @sagikazarmark to join the discussion, and see what he thinks. I would suggest to break dependencies. I know there are places in the library that still use floats for calculations; they should be removed in this cycle too. We could indeed branch off to a 3.x branch.

Moreover I want to discuss how we should store numbers internally. There is this Number object I created which is instantiated too many times I believe, right? (forgive me, it's been a long time I really worked on this lib). We might want to change this.

[Ocramius]

Working on this again today - will consider the changes as if I were on v4, so there will be BC breaks.

There is this Number object I created which is instantiated too many times I believe, right? (forgive me, it's been a long time I really worked on this lib). We might want to change this.

Yes, overall something I'd like to change: use Number only when user input is involved, while operating with string internally at all times, when possible.

[frederikbosch]

Wonderful. My suggestion is to go for PHP8.0 and above, but I hope @sagikazarmark has the opportunity to confirm my suggestion.

[Ocramius]

My suggestion is to go for PHP8.0 and above

Works for me, and also makes my life easier with the whole type improvements. I don't think I'll wait for @sagikazarmark's input: that's mostly because I can (like everyone else) only dedicate limited time to this anyway, and I since v3 already supports PHP 7, there is no need to support it in v4.

[frederikbosch]

Alright, go for it then. You are the one willing to spend time on this huge improve, whereas @sagikazarmark and I are the ones that do not have this time, so I guess we are happy with the changes you provide!

[sagikazarmark]

Sorry for being late to the party. Yeah, I think targeting 4.0 with these changes makes sense. I'm not sure about PHP 8.0, unless there is a specific reason (ie. a feature that we want to use, a dependency that only supports 8.0, etc).

I can create a 3.x branch, so we can manage required checks separately. I wonder if submitting these changes in smaller chunks would be possible (eg. PHP upgrades and build changes then every other change). It would make reviews easier and would make sure that subsequent PRs run the new build pipelines.

We should also discuss the fate of the current 4.0 milestone: https://github.com/moneyphp/money/milestone/4
Is there something from there that needs to be done or can we do those once 4.0 is tagged?

Thanks for putting the effort into this @Ocramius!

[Ocramius]

@frederikbosch should be OK for another round

[frederikbosch]

How about merging in? It's so huge that I have to work with it in order to find improvements.

[Ocramius]

How about merging in? It's so huge that I have to work with it in order to find improvements.

Given the previous review, I suggest reviewing the individual (new) commits one-by-one, since I try to keep commit messages as relevant as possible.

[Ocramius]

@frederikbosch just realized that yours may be a typo in the message: from my PoV, this is mergeable, and I have decent confidence in the amount of checks I've added here to avoid major regressions.

Minor regressions are unavoidable, but that's normality in OSS: https://xkcd.com/1172/

[Ocramius]

I'm releasing this on a fork to internally test it on a customer project: IMO this is mergeable here too 👍

[frederikbosch]

Merged, @Ocramius thanks a lot for all your work here. It is much appreciated by us maintainers, but I guess also by many users. I will try my best to come up with a release - first alpha/beta - as soon as possible.

[frederikbosch]

@Ocramius You added roave/infection-static-analysis-plugin to this package in this PR. Yesterday the MSI reached a point below the 78% that you have set. I have no idea how to approach this, other than lowering this number. Is there any advice you can give in this regard?

[Ocramius]

Yesterday the MSI reached a point below the 78% that you have set.

How did it reach it? What was the change? Was there a dependency change?

[frederikbosch]

I added ext-filter and ext-json after running maglnet/composer-require-checker.

[Ocramius]

@frederikbosch do you have a diff somewhere? Perhaps composer update changed something else?

[frederikbosch]

Yes.

[Ocramius]

@frederikbosch that bosh does a general composer update, which also happened to update infection/infection from 0.20.2 to 0.23.0.

Newer versions of infection/infection apply MORE mutators, and therefore need more effort in testing (especially around regular expressions, for which I think new mutators were introduced).

My suggestion is to:

• revert include all extensions in composer.json, even when they are bundled w… #661
• re-introduce the composer.json (only) changes from include all extensions in composer.json, even when they are bundled w… #661, run a composer update nothing, which does NOT alter composer.lock
• configure @dependabot to upgrade individual dependencies one by one, instead of a one-shot composer update

If you need any help with it, I can probably hop on a call this afternoon CEST.

[frederikbosch]

composer update nothing? Never heard of before, very nice! See #663. Thanks, for the comprehensive remarks. I will try to add dependabot later today.

[bcremer]

I'm optimizing an application that deals with lots of MoneyPHP instances and operations (50k+ instances). The MoneyPHP instantiation and operations sum up to a few hundred milliseconds for a web request so we needed to optimize that.

With the benchmarks provided by @Ocramius I discovered that the add and subtract orations are considerable faster with the GmpCalculator. Most other operations are faster with the default BcMathCalculator.

To get the most performance in our application I wrote a delegating calculator that uses both implementations.

/**
 * Optimized `Money\Calculator` that delegates to either 
 * `BcMathCalculator` or `GmpCalculator` depending on the operation.
 *
 * `GmpCalculator` is approx 25% faster for add and subtract.
 * `BcMathCalculator` is faster for all decimal operations.
 */
class DelegatingCalculator implements Calculator
{
    public static function compare(string $a, string $b): int
    {
        return BcMathCalculator::compare($a, $b);
    }

    public static function add(string $amount, string $addend): string
    {
        return GmpCalculator::add($amount, $addend);
    }
    
[...]

Money::registerCalculator(DelegatingCalculator::class);

Sorry for responding in this old PR but maybe some one else finds this useful.

[frederikbosch]

Do you have some benchmarks? Maybe create a PR with a PerformanceOptimizedDelegateCalculator class? Tests are easy since you can simply extend the abstract CalculatorTestCase.