Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ruff: Minor optimizations of list comprehensions, x in set, etc. #7524

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Nov 6, 2023

Changes proposed in this pull request:

@Yay295
Copy link
Contributor

Yay295 commented Nov 6, 2023

The self.mode != "RGB" and self.mode != "L" to self.mode not in {"RGB", "L"} and similar changes don't seem like an improvement to me. It makes the line shorter, but I can't imagine that it's more efficient to check that something is in a set instead of just checking the two values individually.

@cclauss
Copy link
Contributor Author

cclauss commented Nov 6, 2023

https://docs.astral.sh/ruff/rules/repeated-equality-comparison -- Please read Why is this bad? and References


% python3.12

>>> from dis import dis
>>> def a(x):
...     return x == "a" or x == "b" or x == 1
...
>>> dis(a)
  1           0 RESUME                   0

  2           2 LOAD_FAST                0 (x)
              4 LOAD_CONST               1 ('a')
              6 COMPARE_OP              40 (==)
             10 COPY                     1
             12 POP_JUMP_IF_TRUE        12 (to 38)
             14 POP_TOP
             16 LOAD_FAST                0 (x)
             18 LOAD_CONST               2 ('b')
             20 COMPARE_OP              40 (==)
             24 COPY                     1
             26 POP_JUMP_IF_TRUE         5 (to 38)
             28 POP_TOP
             30 LOAD_FAST                0 (x)
             32 LOAD_CONST               3 (1)
             34 COMPARE_OP              40 (==)
        >>   38 RETURN_VALUE
>>> def b(x):
...     return x in {"a", "b", 1}
...
>>> dis(b)
  1           0 RESUME                   0

  2           2 LOAD_FAST                0 (x)
              4 LOAD_CONST               1 (frozenset({1, 'b', 'a'}))
              6 CONTAINS_OP              0
              8 RETURN_VALUE

@Yay295
Copy link
Contributor

Yay295 commented Nov 6, 2023

Python bytecode is still further interpreted, so I wouldn't necessarily assume that one is faster just because it's shorter. I have done some more looking though and found some more tests:
https://stackoverflow.com/questions/28885132/why-is-x-in-x-faster-than-x-x
pylint-dev/pylint#4776

Basically, in is faster (even sometimes for only one value!) because it's done in C instead of in Python (and it also does an identity check), and sets have some optimizations to be fast even when they're small.

@homm
Copy link
Member

homm commented Nov 8, 2023

+1 for frozensets

@radarhere radarhere requested a review from hugovk November 30, 2023 10:45
@hugovk hugovk merged commit 76446ee into python-pillow:main Dec 1, 2023
49 checks passed
@cclauss cclauss deleted the ruff-rules-C4-PERF102-PIE810-PLR branch December 1, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants