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: Implement `PyType_GetBaseByToken()` and `Py_tp_token` slot by neonene · Pull Request #124163 · python/cpython

Conversation

@neonene

@neonene neonene commented Sep 17, 2024 •
edited by github-actions Bot
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

📚 Documentation preview 📚: https://cpython-previews--124163.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
neonene added 2 commits September 17, 2024 17:57
@neonene
Implement the feature
997e602
@neonene
Use it in _ctypes
3f34cda
@neonene
neonene requested review from a team, encukou and markshannon as code owners September 17, 2024 09:10
@bedevere-app bedevere-app Bot added the awaiting review label Sep 17, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Sep 17, 2024
Add PyType_GetBaseByToken function with Py_tp_token slot #124153
Closed
6 tasks done
neonene added 3 commits September 17, 2024 18:27
@neonene
NEWS
8eb1988
@neonene
edit a comment
97cfc14
@neonene
typo
95640c4
@neonene

neonene commented Sep 17, 2024

Copy link
Copy Markdown
Contributor Author

I'll check this and PoC version again.
All reactions

Sorry, something went wrong.

Uh oh!

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

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

Looks great, thank you!
I have just one style suggestion. But if you'd like to keep it we can merge the PR as it is.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/typeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Sep 17, 2024
@neonene
cleanup
ef49488
neonene
neonene commented Sep 17, 2024
View reviewed changes
Comment thread Objects/typeobject.c Outdated
return -1;
}

if (!_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) {

@neonene neonene Sep 17, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

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.

When I observed a good performance, the line had a typo (missing a NOT), though check_base_by_token() was unused during a PGO test. The telco got slower by 5-10% once I corrected, which was actually fixed by one of the following:

  • Use if-else statement

  • Replace _PyType_HasFeature() with the public PyType_HasFeature(), not with ineffective (type->tp_flags & Py_TPFLAGS_HEAPTYPE)

They are not used in PyType_GetBaseByToken().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
neonene added 4 commits September 18, 2024 05:43
@neonene
fix possible perf regression
afd55a7
@neonene
rely only on if-else statements to fix the perf
254c2e5 
This chains the branches in PyType_GetBaseByToken() as well, which is effective when MSVC is in better condition on PGO.
@neonene
remove a comment
fd8b40d
@neonene
Merge branch 'main' into tp_token
d1d71e3
@neonene

neonene commented Sep 18, 2024

Copy link
Copy Markdown
Contributor Author

I think I've finished. When checking a type without a result, this may be a bit slower than the draft PR, not trained well for now.
encukou 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.
@encukou
encukou merged commit 646f16b into python:main Sep 18, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 18, 2024
@neonene
neonene deleted the tp_token branch September 18, 2024 15:02
@zware zware mentioned this pull request Sep 18, 2024
Fix make htmllive target #124219
Merged
savannahostrowski pushed a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024
@neonene @savannahostrowski
pythongh-124153: Implement PyType_GetBaseByToken() and `Py_tp_token…
6da8650 
…` slot (pythonGH-124163)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou approved these changes
@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

None yet

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.

2 participants

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