GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-72902: improve Fraction(str) speed (don't use regexp's) by skirpichev · Pull Request #133994 · python/cpython

Conversation

@skirpichev

@skirpichev skirpichev commented May 14, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Fractions instantiation revisited #72902

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot mentioned this pull request May 14, 2025
Fractions instantiation revisited #72902
Open
@skirpichev
skirpichev marked this pull request as ready for review May 14, 2025 11:01
@bedevere-app bedevere-app Bot added the awaiting review label May 14, 2025
@skirpichev
skirpichev requested a review from serhiy-storchaka May 14, 2025 11:01
eendebakpt
eendebakpt reviewed May 14, 2025
View reviewed changes
Comment thread Lib/fractions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@eendebakpt

This comment was marked as resolved.

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

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka

This comment was marked as resolved.

Sign in to view

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

This comment was marked as resolved.

Sign in to view
@skirpichev

This comment was marked as resolved.

Sign in to view
@skirpichev
skirpichev marked this pull request as draft May 14, 2025 15:10
@bedevere-app bedevere-app Bot removed the awaiting review label May 14, 2025
skirpichev and others added 3 commits May 16, 2025 06:37
@skirpichev Wolfgang Maier
pythongh-72902: improve Fraction(str) speed (don't use regexp's)
b45b672 
Co-authored-by: Wolfgang Maier <[email protected]>
@skirpichev @eendebakpt
Update Lib/fractions.py
d6b0083 
Co-authored-by: Pieter Eendebak <[email protected]>
@skirpichev
+ a quick fix
05e9110
@skirpichev
skirpichev force-pushed the speedup-Fraction-fromstr/72902 branch from 3687e34 to 05e9110 Compare May 16, 2025 03:37
@skirpichev
skirpichev marked this pull request as ready for review May 16, 2025 03:53
@bedevere-app bedevere-app Bot added the awaiting review label May 16, 2025
@skirpichev

This comment was marked as resolved.

Sign in to view
@skirpichev
skirpichev requested review from eendebakpt and serhiy-storchaka May 16, 2025 03:59
eendebakpt
eendebakpt reviewed May 16, 2025
View reviewed changes
Comment thread Lib/fractions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev @eendebakpt
Update Lib/fractions.py
7c567fc 
Co-authored-by: Pieter Eendebak <[email protected]>
eendebakpt
eendebakpt approved these changes May 18, 2025
View reviewed changes

@eendebakpt eendebakpt left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels May 18, 2025
skirpichev added 2 commits May 19, 2025 05:39
@skirpichev
Merge branch 'master' into speedup-Fraction-fromstr/72902
cb679a0
@skirpichev
address review: add test
e383bdb
@skirpichev

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev
Merge branch 'master' into speedup-Fraction-fromstr/72902
a617212
@skirpichev
skirpichev marked this pull request as draft May 20, 2025 07:56
@bedevere-app bedevere-app Bot removed the awaiting core review label May 20, 2025
skirpichev added 2 commits May 20, 2025 12:01
@skirpichev
+ revert moving numbers.Rational check
48f73f0
@skirpichev
+1
20436d8
@skirpichev

skirpichev commented May 20, 2025

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev
skirpichev marked this pull request as ready for review May 20, 2025 09:13
@bedevere-app bedevere-app Bot added the awaiting review label May 20, 2025
@github-actions

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 24, 2026
@skirpichev skirpichev closed this Apr 24, 2026
@skirpichev
skirpichev deleted the speedup-Fraction-fromstr/72902 branch April 24, 2026 23:03
@eendebakpt eendebakpt mentioned this pull request Apr 26, 2026
gh-72902: improve Fraction(str) speed #149024
Open
@skirpichev
skirpichev restored the speedup-Fraction-fromstr/72902 branch May 23, 2026 07:42
@skirpichev skirpichev reopened this May 23, 2026
@read-the-docs-community

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

  • about.html
  • bugs.html
  • glossary.html
  • index.html
  • license.html
  • py-modindex.html
  • c-api/allocation.html
  • c-api/apiabiversion.html
  • c-api/arg.html
  • c-api/buffer.html
  • and 436 more...

- Deleted

  • improve-page-nojs.html
  • improve-page.html
  • _static/tachyon-example-flamegraph.html
  • _static/tachyon-example-heatmap.html
  • c-api/curses.html
  • c-api/extension-modules.html
  • c-api/interp-lifecycle.html
  • c-api/lifecycle.html
  • c-api/picklebuffer.html
  • c-api/profiling.html
  • and 26 more...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@skirpichev
Merge branch 'main' into speedup-Fraction-fromstr/72902
9d68d36
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label May 23, 2026
skirpichev added 3 commits May 24, 2026 05:37
@skirpichev
Merge branch 'main' into speedup-Fraction-fromstr/72902
aba4573
@skirpichev
Merge branch 'main' into speedup-Fraction-fromstr/72902
68b6e66
@skirpichev
Merge branch 'main' into speedup-Fraction-fromstr/72902
9c57805
@skirpichev skirpichev closed this Sep 25, 2026
@skirpichev
skirpichev deleted the speedup-Fraction-fromstr/72902 branch September 25, 2026 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@skirpichev @eendebakpt @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.