FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-72902: improve Fraction(str) speed (don't use regexp's) by skirpichev · Pull Request #133994 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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

Closed
skirpichev wants to merge 13 commits into
python:mainfrom
skirpichev:speedup-Fraction-fromstr/72902
ConversationCommits (13)ChecksFiles changed
Closed

gh-72902: improve Fraction(str) speed (don't use regexp's)#133994
skirpichev wants to merge 13 commits into
python:mainfrom
skirpichev:speedup-Fraction-fromstr/72902

Conversation

skirpichev commented May 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

For reviewers. I'm not sure if it worth (code seems more complex, IMO), but speedup for string parsing seems noticeable (1.3-1.5x).

Benchmark ref patch
Fraction('123') 19.4 us 15.3 us: 1.27x faster
Fraction('1/3') 19.7 us 12.6 us: 1.56x faster
Fraction('1.2e-3') 26.5 us 19.9 us: 1.33x faster
Fraction('-.2') 23.6 us 18.1 us: 1.30x faster
Geometric mean (ref) 1.36x faster
Details 
# bench.py
import pyperf
from fractions import Fraction as F

runner = pyperf.Runner()
s = 'Fraction'
for v in ["123", "1/3", "1.2e-3", "-.2"]:
    r = s + '(' + repr(v) + ')'
    runner.bench_func(r, F, v)

bedevere-app Bot mentioned this pull request May 14, 2025
Open
skirpichev marked this pull request as ready for review May 14, 2025 11:01
eendebakpt reviewed May 14, 2025
View reviewed changes
Comment thread Lib/fractions.py Outdated

This comment was marked as resolved.

Sign in to view
eendebakpt reviewed May 14, 2025
View reviewed changes
Comment thread Lib/fractions.py

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view
skirpichev marked this pull request as draft May 14, 2025 15:10
skirpichev and others added 3 commits May 16, 2025 06:37
pythongh-72902: improve Fraction(str) speed (don't use regexp's)
b45b672 
Co-authored-by: Wolfgang Maier <wolfgang.maier@biologie.uni-freiburg.de>
Update Lib/fractions.py
d6b0083 
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
+ a quick fix
05e9110
skirpichev force-pushed the speedup-Fraction-fromstr/72902 branch from 3687e34 to 05e9110 Compare May 16, 2025 03:37
skirpichev marked this pull request as ready for review May 16, 2025 03:53

This comment was marked as resolved.

Sign in to view
eendebakpt reviewed May 16, 2025
View reviewed changes
Comment thread Lib/fractions.py Outdated
Update Lib/fractions.py
7c567fc 
Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>
eendebakpt approved these changes May 18, 2025
View reviewed changes

eendebakpt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@skirpichev Could you also add a test case for Fraction('232e\t2') (this should fail). One optimization I tried passes all the tests, but fails on this one.

Tests pass, and all paths in the new code are needed, so approving. The new code is not very easy to read, but neither is the regular expression that is currently in main.

skirpichev commented May 19, 2025

Copy link
Copy Markdown
Member Author

Could you also add a test case for Fraction('232e\t2') (this should fail).

Done.

The new code is not very easy to read, but neither is the regular expression that is currently in main.

I think that the later is more clear. Also, it has more nice tracebacks.

So, I'm still not sure about this. Speed gain is x2 at most. Maybe regexps can be eventually optimized?

serhiy-storchaka commented May 19, 2025

Copy link
Copy Markdown
Member

I was surprised that manual parsing is faster than a regexp-based approach. The regular expression is not optimal, so I tried to optimize it, but the benefit was minuscule. The regexp engine needs to be optimized.

Can we please wait a few months with these changes while I try to optimize the regexp engine for such case? This seems so typical that a lot of code would benefit from optimization.

skirpichev commented May 19, 2025

Copy link
Copy Markdown
Member Author

I was surprised that manual parsing is faster than a regexp-based approach.

But not too much.

The regexp engine needs to be optimized.
Can we please wait a few months with these changes while I try to optimize the regexp engine for such case?

Sure. I'm not familiar with the regexp module code and I would be interested in such fix, if it's possible at all. BTW, so far this pr was not approved by core dev.

@serhiy-storchaka, but what about other change, i.e. moving down numbers.Rational check? Maybe I should factor out this to a separate pr? See benchmarks (third column) and my arguments for this.

skirpichev marked this pull request as draft May 20, 2025 07:56

skirpichev commented May 20, 2025

Copy link
Copy Markdown
Member Author

Ok, now this has only optimization for str inputs. Other goes to #134320

skirpichev marked this pull request as ready for review May 20, 2025 09:13

github-actions Bot commented Apr 24, 2026

Copy link
Copy Markdown

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

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 24, 2026
skirpichev closed this Apr 24, 2026
skirpichev deleted the speedup-Fraction-fromstr/72902 branch April 24, 2026 23:03
eendebakpt mentioned this pull request Apr 26, 2026
Open
skirpichev restored the speedup-Fraction-fromstr/72902 branch May 23, 2026 07:42
skirpichev reopened this May 23, 2026

read-the-docs-community Bot commented May 23, 2026

Copy link
Copy Markdown

Documentation build overview

📚 cpython-previews | 🛠️ Build #32820273 | 📁 Comparing 20436d8 against main (a7d5a6c)

  🔍 Preview build  

482 files changed · ± 446 modified · - 36 deleted

± Modified

- Deleted

github-actions Bot removed the stale Stale PR or inactive for long period of time. label May 23, 2026
skirpichev closed this Sep 25, 2026
skirpichev deleted the speedup-Fraction-fromstr/72902 branch September 25, 2026 12:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

eendebakpt eendebakpt approved these changes
serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL