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-124688: _decimal: Get a module state from ctx objects for performance by neonene · Pull Request #124691 · python/cpython

Conversation

@neonene

@neonene neonene commented Sep 27, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: _decimal: Store a module state in objects for faster access #124688

Sorry, something went wrong.

Uh oh!

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

All reactions
neonene added 4 commits September 28, 2024 02:27
@neonene
add a getter function
7a9fac1
@neonene
add _dec_mpd_radix()
818c24f
@neonene
PyDecType_New, dec_alloc, dec_from_long
00d2322
@neonene
replace related get_module_state_by_def()
ab5adad
@neonene
neonene requested a review from rhettinger as a code owner September 27, 2024 18:01
@bedevere-app bedevere-app Bot added the awaiting review label Sep 27, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Sep 27, 2024
_decimal: Store a module state in objects for faster access #124688
Closed
@vstinner

vstinner commented Sep 27, 2024

Copy link
Copy Markdown
Member

Do you have a benchmark showing the effect of this change?

All reactions

Sorry, something went wrong.

Uh oh!

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

@neonene

neonene commented Sep 27, 2024

Copy link
Copy Markdown
Contributor Author

_dec_mpd_radix() is added (818c24f) so as not to be passed a different object, not for performance.

All reactions

Sorry, something went wrong.

Uh oh!

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

@rhettinger
rhettinger removed their request for review September 27, 2024 18:15
@neonene

neonene commented Sep 27, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

My different PGO builds says 3-5% faster on the telco benchmark.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Sep 27, 2024

Copy link
Copy Markdown
Member

My different PGO builds says 3-5% faster on the telco benchmark.

Would you mind to share your results (before/after, difference)?

All reactions

Sorry, something went wrong.

Uh oh!

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

@neonene

neonene commented Sep 27, 2024

Copy link
Copy Markdown
Contributor Author

On release builds, the result was as fast as the global state access: #123100 (comment) (PR with alternatives: 1.08x )

All reactions

Sorry, something went wrong.

Uh oh!

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

@neonene

neonene commented Sep 27, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Picked a few telco results on Windows PGO builds:

a) main with d69529d restored (hard to evaluate main as-is)

9.68 ms +- 0.17 ms -> 8.94 ms +- 0.15 ms: 1.08x faster  # PR (ctx object)
9.68 ms +- 0.17 ms -> 8.76 ms +- 0.11 ms: 1.10x faster  # global state access

b) (a) with some patches

8.99 ms +- 0.11 ms -> 8.49 ms +- 0.16 ms: 1.06x faster  # PR (ctx object)
8.99 ms +- 0.11 ms -> 8.44 ms +- 0.20 ms: 1.07x faster  # global state access

c) (b) with 1 diff in PyType_GetBaseByToken()

8.91 ms +- 0.21 ms -> 8.78 ms +- 0.16 ms: 1.02x faster  # PR (ctx object)
8.91 ms +- 0.21 ms -> 8.25 ms +- 0.17 ms: 1.08x faster  # global state access
All reactions

Sorry, something went wrong.

Uh oh!

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

@neonene
delete some spaces
a34f2d3
@vstinner

vstinner commented Sep 28, 2024

Copy link
Copy Markdown
Member

Sorry, I don't know how to read your 6 benchmarks. I don't understand what you compared. I don't know what are "some patches" nor why you mention PyType_GetBaseByToken(), it doesn't help me to compare this PR.

So I just compared the main branch to this PR:

Mean +- std dev: [ref] 8.18 ms +- 0.28 ms -> [change] 7.53 ms +- 0.18 ms: 1.09x faster

Nice! 1.09x faster is worth it.

neonene reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

vstinner
vstinner reviewed Sep 28, 2024
View reviewed changes
Comment thread Modules/_decimal/_decimal.c Resolved

Uh oh!

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

@vstinner
Update Modules/_decimal/_decimal.c
f7dd655
@vstinner
vstinner enabled auto-merge (squash) September 28, 2024 15:03
@picnixz picnixz added the skip news label Sep 28, 2024
@vstinner
vstinner merged commit dc12237 into python:main Sep 28, 2024
@bedevere-app bedevere-app Bot removed the awaiting review label Sep 28, 2024
@picnixz

picnixz commented Sep 28, 2024

Copy link
Copy Markdown
Member

I added the skip news since Victor enabled auto-merge. If a NEWS entry is really needed, we can add it later in a separate commit though.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Sep 28, 2024

Copy link
Copy Markdown
Member

Merged, thanks @neonene!

All reactions

Sorry, something went wrong.

Uh oh!

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

@neonene
neonene deleted the decstate branch September 28, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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

@neonene @vstinner @picnixz
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 01402503 — 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.