Improve performance of multipliedBy one if value is one

[tomtomsen]

Currently there is only a check if $that->value is one.
With this patch $this->value will be checked too.

[BenMorel]

Thanks, looking good!

Could you please apply the same optimization to BigDecimal::multipliedBy()?

Also, it looks like plus() and minus() in both classes could benefit from a similar optimization by returning $that under the right circumstances.

[BenMorel]

Not sure about this one: the optimizations above are really to return one of the objects without modification whenever possible, but for the other micro-optimizations, they're already handled by the calculator if needed.

I don't think this extra check brings much value.

[BenMorel]

Thanks for your work here! This is overall very good, I'm just a bit torn regarding everything that doesn't return $this or $that unmodified.

The aim of these optimizations is mainly to avoid creating a new object when this is not necessary. From this perspective, returning BigInteger::zero() may also be acceptable, as we're returning a cached object.

However, for everything else, like returning ->negated() or new BigDecimal(), I don't think that these optimizations bring much value as the underlying operation will be very quick anyway in this case, even with the NativeCalculator.

In other words, out of your 9 additions:

• 4 are perfect to me (return $this or $that)
• 2 are OK-ish (return BigInteger::zero())
• 3 are, arguably, unnecessary bloat (return $that->negated() and return new BigDecimal)

What's your take on this?

[BenMorel]

No feedback here?

[jkuchar]

Perfectly makes sense to me @BenMorel, from my perspective -- taking always balance between code complexity&readability and speed. I would take first 4 as these are only one which any possible measurable speed difference.

[BenMorel]

@tomtomsen I guess we'll just keep the 4x return $that; for now. Can you please change your PR accordingly?

You can keep all added tests, even if some of some were added to cover the other use cases. They don't hurt.

[tomtomsen]

@BenMorel Yes, I will update the pull request.

[BenMorel]

LGTM!

Don't worry, I'm not sure whether your other optimizations are "over the top", it's just hard to benchmark whether they're relevant or not, and whether they justify extra code complexity that also affects readability. For now your 4 new tests are a nice addition. Thank you.

[BenMorel]

Released as 0.8.9.