Skip to content

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