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

gh-100239: specialize dict subclasses with no getitem override by iritkatriel · Pull Request #132383 · python/cpython · GitHub

/ cpython Public

gh-100239: specialize dict subclasses with no getitem override - #132383

Closed
iritkatriel wants to merge 6 commits into
python:mainfrom
iritkatriel:dicts
ConversationCommits (6)ChecksFiles changed
Closed

gh-100239: specialize dict subclasses with no getitem override#132383
iritkatriel wants to merge 6 commits into
python:mainfrom
iritkatriel:dicts

Conversation

iritkatriel commented Apr 11, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

bedevere-app Bot mentioned this pull request Apr 11, 2025
Open
iritkatriel changed the title gh-100239: spcialize dict subclasses with no getitem override gh-100239: specialize dict subclasses with no getitem override Apr 11, 2025

iritkatriel commented Apr 11, 2025

Copy link
Copy Markdown
Member Author

The tests will be great again once we merge the fix for #132284.

kumaraditya303 reviewed Apr 12, 2025
View reviewed changes
Comment thread Python/bytecodes.c Outdated
PyObject *dict = PyStackRef_AsPyObjectBorrow(dict_st);

assert(PyDict_CheckExact(dict));
assert(PyDict_Check(dict));

kumaraditya303 Apr 12, 2025

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

I think you also need to change _GUARD_NOS_DICT to PyDict_Check instead of PyDict_CheckExact for this to work but I am not sure.

iritkatriel Apr 12, 2025

Copy link
Copy Markdown
Member Author

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

It does look like it from the code, but I don't see more misses.

iritkatriel commented Apr 12, 2025

Copy link
Copy Markdown
Member Author

Perf with the fix to the guard: https://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20250412-3.14.0a7%2B-3d9cf36/README.md

markshannon reviewed Apr 14, 2025
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

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

Is it worth making dict_subscript non-static to avoid the indirection PyDict_Type.tp_as_mapping->mp_subscript?

Comment thread Python/bytecodes.c Outdated
iritkatriel requested a review from methane as a code owner April 14, 2025 15:32
markshannon reviewed Apr 15, 2025
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

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

It looks like the test_opcode test needs updating.

Comment thread Python/bytecodes.c Outdated
Comment thread Python/specialize.c Outdated
kumaraditya303 mentioned this pull request Apr 5, 2026
Merged

github-actions Bot commented Apr 23, 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 23, 2026

kumaraditya303 commented May 29, 2026

Copy link
Copy Markdown
Contributor

Superseded by #148128

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

markshannon markshannon left review comments
kumaraditya303 kumaraditya303 left review comments
methane methane approved these changes

Assignees

No one assigned

Labels

awaiting merge skip news stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL