Skip to content

Enable Ruff PERF - #13312

Closed
Avasam wants to merge 4 commits into
python:mainfrom
Avasam:Ruff-PERF
Closed

Enable Ruff PERF#13312
Avasam wants to merge 4 commits into
python:mainfrom
Avasam:Ruff-PERF

Conversation

@Avasam

@AvasamAvasam commented Dec 26, 2024

Copy link
Copy Markdown
Collaborator

Ref #13295
https://docs.astral.sh/ruff/rules/#perflint-perf

That manual-list-comprehension (PERF401)initially looked like a false-positive I just had to add a parenthesis and make everything list comprehensions. I'll let the numbers talk for themselves in comments.

About try-except-in-loop (PERF203), here's prior conversation with @AlexWaygood : astral-sh/ruff#12805 (comment)

Comment on lines 93 to 103
# that are most likely to point to the source code first
urls_to_check: list[str] = []
url_names_probably_pointing_to_source = ("Source", "Repository", "Homepage")

urls_to_check: list[str] = []
for url_name in url_names_probably_pointing_to_source:
if url := project_urls.get(url_name):
urls_to_check.append(url)
urls_to_check.append(url) # noqa: PERF401 # False-positive with walrus, this code is the fastest form
urls_to_check.extend(
url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source
[url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source]
)

for url in urls_to_check:

@AvasamAvasamDec 26, 2024

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Test:

importtimeitsetup="""from itertools import chainproject_urls = { "Changelog": "https://code.qt.io/cgit/pyside/pyside-setup.git/tree/doc/changelogs", "Documentation": "https://doc.qt.io/qtforpython", "Homepage": "https://pyside.org", "Repository": "https://code.qt.io/cgit/pyside/pyside-setup.git/", "Tracker": "https://bugreports.qt.io/projects/PYSIDE",}url_names_probably_pointing_to_source = ("Source", "Repository", "Homepage")"""stmt="""urls_to_check: list[str] = []for url_name in url_names_probably_pointing_to_source: if url := project_urls.get(url_name): urls_to_check.append(url)urls_to_check.extend( url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source)for url in urls_to_check: pass"""print(timeit.timeit(stmt=stmt, setup=setup))
stmt="""urls_to_check: list[str] = []for url_name in url_names_probably_pointing_to_source: if url := project_urls.get(url_name): urls_to_check.append(url)urls_to_check.extend( [url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source])for url in urls_to_check: pass"""print(timeit.timeit(stmt=stmt, setup=setup))
stmt="""project_urls_probably_pointing_to_source = ( project_urls.get(url_name) for url_name in url_names_probably_pointing_to_source)urls_to_check = chain( (url for url in project_urls_probably_pointing_to_source if url), (url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source),)for url in urls_to_check: pass"""print(timeit.timeit(stmt=stmt, setup=setup))
stmt="""project_urls_probably_pointing_to_source = [project_urls.get(url_name) for url_name in url_names_probably_pointing_to_source]urls_to_check = ( [url for url in project_urls_probably_pointing_to_source if url] + [url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source])for url in urls_to_check: pass"""print(timeit.timeit(stmt=stmt, setup=setup))

Results (Python 3.10):

0.68677179999940560.6101396999983990.92887759999939590.7600129999991623

Results (Python 3.13):

0.74357270000109570.53912350000246080.99498630000016420.5781141000006755

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Nevermind, I was just missing parenthesis: astral-sh/ruff#15047 (comment)
(new run time on python 3.13 is 0.5153441999973438)

@hauntsaninjahauntsaninja left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is harder to read. In general, I'm not sure we need this level of linting on typeshed

Comment on lines +94 to +96
urls_to_check = [url for url_name in url_names_probably_pointing_to_source if (url := project_urls.get(url_name))] + [
url for url_name, url in project_urls.items() if url_name not in url_names_probably_pointing_to_source
)
]

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I'd prefer formatting as follow but Black doesn't allow:

urls_to_check= (
[urlforurl_nameinurl_names_probably_pointing_to_sourceif (url:=project_urls.get(url_name))]
+ [urlforurl_name, urlinproject_urls.items() ifurl_namenotinurl_names_probably_pointing_to_source]
)

@AlexWaygood

Copy link
Copy Markdown
Member

I don't feel like anything in typeshed is performance-critical such that we need to worry about micro-optimising specific functions in our tests; I agree that these rules don't really make sense for this repository

@AlexWaygood

Copy link
Copy Markdown
Member

From the comments and upvotes here, I think there's sufficient opposition from the core team here that we probably won't be going ahead with this one :-)

@Avasam
Avasam deleted the Ruff-PERF branch January 2, 2025 22:19
Sign up for freeto 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.

3 participants

@Avasam@AlexWaygood@hauntsaninja