Skip to content

gh-72902: improve Fraction(str) speed (don't use regexp's) #133994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented May 14, 2025


For reviewers. I'm not sure if it worth (code seems more complex, IMO), but speedup for string parsing seems noticeable (1.5-2x). Partially due to moving down branch with numbers.Rational check (IMO, that does make sense), but mostly due to removing regexps.

Benchmark ref patch
Fraction(myint) 5.84 us 7.18 us: 1.23x slower
Fraction('123') 19.2 us 10.1 us: 1.89x faster
Fraction('1/3') 19.7 us 9.53 us: 2.06x faster
Fraction('1.2e-3') 26.4 us 15.2 us: 1.74x faster
Fraction('-.2') 23.4 us 14.4 us: 1.63x faster
Geometric mean (ref) 1.55x faster
# bench.py
import pyperf
from fractions import Fraction as F
from numbers import Integral

runner = pyperf.Runner()
s = 'Fraction'
class myint:
    numerator = 123
    denominator = 1
    def __int__(self):
        return 123
    def __repr__(self):
        return "myint"
Integral.register(myint)
for v in [myint(), "123", "1/3", "1.2e-3", "-.2"]:
    r = s + '(' + repr(v) + ')'
    runner.bench_func(r, F, v)

@skirpichev skirpichev marked this pull request as ready for review May 14, 2025 11:01
Co-authored-by: Pieter Eendebak <[email protected]>
@eendebakpt
Copy link
Contributor

The added code complexity is not too bad imo. But how often is the fraction parsing of strings a bottleneck? (i have no good answer here). And how import is Fraction(fraction_instance)? (that gets a little bit slower)

There a few corner cases where behavior is different. For example in this PR Fraction('+ 343.33') is valid, where before it was rejected. Can you add tests for those? (and maybe change the implementation to reject?)

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The large part of the speed up is due to moving the Rational check. This makes Fraction -> Fraction slower.

We must put correctness ahead of speed. Fraction('0x10/0b101')

@serhiy-storchaka
Copy link
Member

See #134010 which adds new tests.

@skirpichev
Copy link
Member Author

But how often is the fraction parsing of strings a bottleneck?
[...]
The large part of the speed up is due to moving the Rational check. This makes Fraction -> Fraction slower.

  1. The largest speed up is not due to moved check. (Else this was in benchmarks.)
  2. I don't think that idempotence is something that can be a bottleneck. Why you want Fraction(Fraction)? (Certainly, this is not a hot path for the fractions module itself, if used at all.) Fraction(Rational) seems a valid use case for me, but less common than Fraction(str).

There a few corner cases where behavior is different. For example in this PR Fraction('+ 343.33') [...]
We must put correctness ahead of speed. Fraction('0x10/0b101')

Thanks, good points. I'll try to fix this.

new tests.

Seems to be a good outcome of the original issue for me.

@skirpichev skirpichev marked this pull request as draft May 14, 2025 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants