Skip to content

Conversation

ambv
Copy link
Contributor

@ambv ambv commented Sep 15, 2025

@ambv ambv force-pushed the gh-134953-followup branch from 04f98d4 to 8e85d33 Compare September 15, 2025 15:13
@ambv ambv added the skip news label Sep 15, 2025
Copy link
Member

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

LGTM

ZERO_WIDTH_BRACKET = re.compile(r"\x01.*?\x02")
ZERO_WIDTH_TRANS = str.maketrans({"\x01": "", "\x02": ""})
IDENTIFIERS_AFTER = {"def", "class"}
KEYWORD_CONSTANTS = {"True", "False", "None"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
KEYWORD_CONSTANTS = {"True", "False", "None"}
KEYWORD_CONSTANTS = frozenset({"True", "False", "None"})

Is even faster.

Copy link
Member

Choose a reason for hiding this comment

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

The compiler already does this automatically:

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I created a bit more general issue: #139003. For _pyrepl I'll make a PR.

@ambv ambv merged commit 811acc8 into python:main Sep 15, 2025
53 checks passed
@ambv ambv added the needs backport to 3.14 bugs and security fixes label Sep 15, 2025
@miss-islington-app
Copy link

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
…nGH-138931)

(cherry picked from commit 811acc8)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
@bedevere-app
Copy link

bedevere-app bot commented Sep 15, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants