gh-91851: Micro optimizations for Fraction's arithmetic

[skirpichev]

This apply some suggestions from the #24779 review.

I've tested also direct imports (e.g. from math import gcd), suggested by @tim-one, but the difference doesn't seems to be noticeable.

Resolves #91851

• Issue: Optimize the Fraction class arithmetics for small components #91851

Benchmarks (taken from the second commit msg):

That makes arithmetics for small components just as fast
as before #24779, except for a mixed case (e.g.
Fraction + int):

On master:

  $ ./python -m timeit -s 'from fractions import Fraction as F' \
    -s 'a = F(7, 8)' -s 'b = F(3, 4)' 'a + b'
  20000 loops, best of 5: 12.4 usec per loop
  $ ./python -m timeit -s 'from fractions import Fraction as F' \
    -s 'a = F(7, 8)' -s 'b = 3' 'a + b'
  20000 loops, best of 5: 10.3 usec per loop
  $ ./python -m timeit -s 'from fractions import Fraction as F' \
    -s 'a = 3' -s 'b = F(3, 4)' 'a + b'
  20000 loops, best of 5: 12.6 usec per loop

With the patch:

  $ ./python -m timeit -s 'from fractions import Fraction as F' \
    -s 'a = F(7, 8)' -s 'b = F(3, 4)' 'a + b'
  20000 loops, best of 5: 9.98 usec per loop
  $ ./python -m timeit -s 'from fractions import Fraction as F' \
    -s 'a = F(7, 8)' -s 'b = 3' 'a + b'
  20000 loops, best of 5: 15.6 usec per loop
  $ ./python -m timeit -s 'from fractions import Fraction as F' \
    -s 'a = 3' -s 'b = F(3, 4)' 'a + b'
  20000 loops, best of 5: 16.6 usec per loop

[github-actions]

This PR is stale because it has been open for 30 days with no activity.

[hauntsaninja]

Thanks for the PR! Could you share some benchmarks? When I run it, this seems to speed up adding a Fraction to a Fraction by 1.25x, but results in a 0.6x slowdown for adding a Fraction to an int.

[skirpichev]

@hauntsaninja, actually there were benchmarks (second commit message). Now copied to the pr's body, thanks.

[eendebakpt]

@hauntsaninja, actually there were benchmarks (second commit message). Now copied to the pr's body, thanks.

@skirpichev @hauntsaninja The PR indeed makes addition of Fraction and int slower. The regression is in this https://github.com/python/cpython/pull/25518/files#diff-f561eb7eb97f832b2698837f52c2c2cf23bdb0b31c69cf1f6aaa560280993316R398 change, which forces the int types to convert from int to Fraction. In the current main that converstion is not required since the int type has properties .denominator and .numerator just like the Fraction type. Reverting that change would force all all monomorphic_operator functions to make only use of .numerator and .denominator (and not of ._denominator). I do not see how to make the construction of Fraction from int much faster, there is already a fast pass in the Fraction constructor.

The optimizations in __round__, __ceil__ and __floor__ are still good.

[eendebakpt]

Also here https://github.com/python/cpython/pull/25518/files#diff-f561eb7eb97f832b2698837f52c2c2cf23bdb0b31c69cf1f6aaa560280993316R414 there is a regression for the case 1+Fraction(2) and other cases r+Fraction(2) where r is of Rational type.

[skirpichev]

@eendebakpt, arithmetic optimization assume that the hot path is Fraction op Fraction. I don't see how to avoid speed regression here. Perhaps, only with the cost of using public properties for the Rational subtypes and private - only for the Fraction type. Any thoughts?

The optimizations in round, ceil and floor are still good.
Should I create a separate pr to get this merged?

[hauntsaninja]

If you create a separate PR for round, ceil and floor, I can merge it since that's uncontroversial :-)

[skirpichev]

@hauntsaninja , see #100791

[hauntsaninja]

Thanks for splitting off the other PR and rebasing this one. I don't have good enough insight into fractions to know if this tradeoff is worth it, e.g. if Fraction + int is as common as Fraction + Fraction, this is not a win.

[eendebakpt]

The change to use properties for numerator and deniminator was done 15 years ago. From the comments I gather the reason was to make them read-only to prevent users from changing them. Reverting that change to make this PR faster would break backwards compatibility.

[mdickinson]

The changes LGTM - I find the new code to be cleaner (as well as fast for the Fraction op Fraction case).

[skirpichev]

@eendebakpt

The change to use properties for numerator and deniminator was done 15 years ago.

It seems, 7736b5b is that commit, correct? I don't see mentioned comments in the related discussion (#46023).

break backwards compatibility

Could you give an example?

[eendebakpt]

@eendebakpt

The change to use properties for numerator and deniminator was done 15 years ago.

It seems, 7736b5b is that commit, correct? I don't see mentioned comments in the related discussion (#46023).

break backwards compatibility

Could you give an example?

The commit is 400adb0

Such a change would impact subclasses of Fraction that directly set '._numerator' . I am not aware of such subclasses, but they can exist.

(to be clear: this PR is fine)

[hauntsaninja]

Thanks for the PR! And yes, definitely nice that monomorphic_operator is now more monomorphic

[skirpichev]

Thanks to all for reviews.

Such a change would impact subclasses of Fraction that directly set '._numerator' .

@eendebakpt You can do this as long as you respect constraints on these private attributes. I.e. they are essentially numerator/denominator properties of the numbers.Rational ("The numerator and denominator values should be instances of Integral and should be in lowest terms with denominator positive."). If you ignore this - some methods of the Fraction class will be broken anyway, starting from the __eq__().

I hardly can imagine benefits of subclassing the Fraction. E.g. if you want to keep components in an unnormalized form - most arithmetic methods should be revised.