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-124153: Remove `_PyType_GetModuleByDef2` private function by neonene · Pull Request #124261 · python/cpython

Conversation

@neonene

@neonene neonene commented Sep 20, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: Add PyType_GetBaseByToken function with Py_tp_token slot #124153

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
neonene added 3 commits September 20, 2024 11:56
@neonene
optimize PyType_GetBaseByToken() dedicatedly
56d3ed1 
Unlike PyType_GetModuleByDef(), this uses PyType_HasFeature() for differentiation.
@neonene
update defdict_or() in _collectionsmodule.c
f7db086
@neonene
replace _PyType_GetModuleByDef2() in _decimal.c
c93c010 
This will not be inlined, but hopefully 4% faster on Windows PGO.  The forceinline version was less efficient (2% faster).
@neonene
neonene requested review from markshannon and rhettinger as code owners September 20, 2024 03:25
@bedevere-app bedevere-app Bot added the awaiting review label Sep 20, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Sep 20, 2024
Add PyType_GetBaseByToken function with Py_tp_token slot #124153
Closed
6 tasks done
@neonene neonene mentioned this pull request Sep 20, 2024
gh-124153: Remove _PyType_GetModuleByDef2 private function #124262
Closed
vstinner
vstinner reviewed Sep 20, 2024
View reviewed changes
Comment thread Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@neonene
leave dec_richcompare() as is
967d197 
PyType_GetModuleByDef() is now slower.
@neonene

This comment was marked as outdated.

Sign in to view
neonene added 2 commits September 20, 2024 20:15
@neonene
keep using _PyType_HasFeature() in loops
8c5fc03
@neonene
roll back typeobject.c to main
af8b92c
@neonene

This comment was marked as outdated.

Sign in to view
@encukou encukou added the skip news label Sep 24, 2024
@encukou

encukou commented Sep 24, 2024

Copy link
Copy Markdown
Member

Looks like this removes the only call to _PyType_GetModuleByDef2; could you also remove the function?
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Sep 24, 2024

Copy link
Copy Markdown
Member

Looks like this removes the only call to _PyType_GetModuleByDef2; could you also remove the function?

@neonene prepared #124262 but this follow-up PR cannot be merged before the code is updated to no longer call _PyType_GetModuleByDef2 ;-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@neonene
remove _PyType_GetModuleByDef2 form header
643f758
@neonene

neonene commented Sep 24, 2024

Copy link
Copy Markdown
Contributor Author

@neonene prepared #124262 but this follow-up PR cannot be merged before the code is updated to no longer call _PyType_GetModuleByDef2 ;-)

I've port the header update from #124262 to here. Is this what you mean?
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Sep 24, 2024

Copy link
Copy Markdown
Member

Why not removing _PyType_GetModuleByDef2() body as well?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@neonene
remove _PyType_GetModuleByDef2() impl
a4e712a
@neonene neonene changed the title gh-124153: Use Py_tp_token slot and PyType_GetBaseByToken() in a few modules gh-124153: Remove _PyType_GetModuleByDef2 private function Sep 24, 2024
@neonene

neonene commented Sep 25, 2024

Copy link
Copy Markdown
Contributor Author

(Does it really make sense to remove _PyType_GetModuleByDef2() before checking the features on main?)
All reactions

Sorry, something went wrong.

Uh oh!

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

encukou commented Sep 25, 2024

Copy link
Copy Markdown
Member

To be honest I don't follow too closely the order in which the changes here should be made/merged.
I also have no way to benchmark with MSVC, so I trust you about the performance, and just review for functionality and readability :)

If this removes the last call to _PyType_GetModuleByDef2, and we don't plan to bring the function back, then it should be removed now. If it turns out to be needed again, we can find it in Git history.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Sep 26, 2024
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

LGTM

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 merge and removed awaiting review labels Sep 26, 2024
@encukou
encukou merged commit d7248cd into python:main Sep 26, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 26, 2024
@neonene
neonene deleted the usetoken branch September 26, 2024 16:51
@neonene neonene mentioned this pull request Sep 26, 2024
PyType_GetModuleByDef family for binary functions performance #117578
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

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