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

gh-134953: Make the True/False/None check more efficient by ambv · Pull Request #138931 · python/cpython · GitHub

/ cpython Public

gh-134953: Make the True/False/None check more efficient - #138931

Merged
ambv merged 1 commit into
python:mainfrom
ambv:gh-134953-followup
Sep 15, 2025
ConversationCommits (1)ChecksFiles changed
Merged

gh-134953: Make the True/False/None check more efficient#138931
ambv merged 1 commit into
python:mainfrom
ambv:gh-134953-followup

Conversation

ambv commented Sep 15, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

bedevere-app Bot mentioned this pull request Sep 15, 2025
Closed
ambv force-pushed the gh-134953-followup branch from 04f98d4 to 8e85d33 Compare September 15, 2025 15:13
ambv added the skip news label Sep 15, 2025
StanFromIreland approved these changes Sep 15, 2025
View reviewed changes

StanFromIreland 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

LGTM

eendebakpt reviewed Sep 15, 2025
View reviewed changes
Comment thread Lib/_pyrepl/utils.py
ZERO_WIDTH_BRACKET = re.compile(r"\x01.*?\x02")
ZERO_WIDTH_TRANS = str.maketrans({"\x01": "", "\x02": ""})
IDENTIFIERS_AFTER = {"def", "class"}
KEYWORD_CONSTANTS = {"True", "False", "None"}

eendebakpt Sep 15, 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
Suggested change
KEYWORD_CONSTANTS = {"True", "False", "None"}
KEYWORD_CONSTANTS = frozenset({"True", "False", "None"})

Is even faster.

pablogsal Sep 15, 2025

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

The compiler already does this automatically:

>>> def foo():
...     KEYWORD_CONSTANTS = {"True", "False", "None"}
...
>>> foo.__code__.co_consts
(None, frozenset({'False', 'True', 'None'}))

auscompgeek Sep 16, 2025

Copy link
Copy Markdown

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

The compiler only does that for function locals, right? A module variable could be mutated from other modules.

eendebakpt Sep 16, 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

@auscompgeek You are right, for module level variables the optimization is not performed, so KEYWORD_CONSTANTS stays a set. For the free-threading build the performance difference is measurable:

Python 3.15.0a0 free-threading build (heads/main-dirty:2a54acf3c3d, Sep  1 2025, 14:33:50) [MSC v.1939 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import timeit
>>> timeit.timeit('[10 in s for ii in range(100)]', setup='from _pyrepl.utils import KEYWORD_CONSTANTS as s; f = frozenset(s)')
8.864965499844402
>>> timeit.timeit('[10 in f for ii in range(100)]', setup='from _pyrepl.utils import KEYWORD_CONSTANTS as s; f = frozenset(s)')
8.371516299899668

ambv Sep 16, 2025

Copy link
Copy Markdown
Contributor 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

@eendebakpt I'll accept a PR to wrap all global sets in _pyrepl.utils as frozensets. There's quite a few of them.

eendebakpt Sep 16, 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 created a bit more general issue: #139003. For _pyrepl I'll make a PR.

ambv merged commit 811acc8 into python:main Sep 15, 2025
53 checks passed
ambv added the needs backport to 3.14 bugs and security fixes label Sep 15, 2025

miss-islington-app Bot commented Sep 15, 2025

Copy link
Copy Markdown

Thanks @ambv for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 15, 2025
pythongh-134953: Make the True/False/None check more efficient (pytho…
569db03 
…nGH-138931)

(cherry picked from commit 811acc8)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>

bedevere-app Bot commented Sep 15, 2025

Copy link
Copy Markdown

GH-138939 is a backport of this pull request to the 3.14 branch.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Sep 15, 2025
eendebakpt mentioned this pull request Sep 16, 2025
Merged
pablogsal pushed a commit that referenced this pull request Sep 29, 2025
[3.14] gh-134953: Make the True/False/None check more efficient (GH-1…
4372a05 
…38931) (#138939)
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

eendebakpt eendebakpt left review comments
StanFromIreland StanFromIreland approved these changes
pablogsal Awaiting requested review from pablogsal pablogsal is a code owner
lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner
+1 more reviewer
auscompgeek auscompgeek left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL