Skip to content

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

Closed
iritkatriel wants to merge 6 commits into
python:mainfrom
iritkatriel:dicts
Closed

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

Conversation

@iritkatriel

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

Copy link
Copy Markdown
Member

@bedevere-appbedevere-appBot mentioned this pull request Apr 11, 2025
Open
@iritkatrieliritkatriel changed the title gh-100239: spcialize dict subclasses with no getitem overridegh-100239: specialize dict subclasses with no getitem overrideApr 11, 2025
@iritkatriel

iritkatriel commented Apr 11, 2025

Copy link
Copy Markdown
MemberAuthor

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

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

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

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

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.

@iritkatrieliritkatrielApr 12, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

image

@iritkatriel

iritkatriel commented Apr 12, 2025

Copy link
Copy Markdown
MemberAuthor

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
markshannon reviewed Apr 14, 2025
View reviewed changes

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

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

Comment threadPython/bytecodes.c Outdated
markshannon
markshannon reviewed Apr 15, 2025
View reviewed changes

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

It looks like the test_opcode test needs updating.

Comment threadPython/bytecodes.c Outdated
Comment threadPython/specialize.c Outdated
@kumaraditya303kumaraditya303 mentioned this pull request Apr 5, 2026
Merged
@github-actions

github-actionsBot commented Apr 23, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Apr 23, 2026
@kumaraditya303

kumaraditya303 commented May 29, 2026

Copy link
Copy Markdown
Contributor

Superseded by #148128

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannonmarkshannonmarkshannon left review comments
@kumaraditya303kumaraditya303kumaraditya303 left review comments
@methanemethanemethane approved these changes

Assignees

No one assigned

Labels

awaiting mergeskip newsstaleStale 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

@iritkatriel@kumaraditya303@methane@markshannon