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

perf(formatter): Skip bodies without comments #4978

Merged
merged 1 commit into from
Jun 9, 2023
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jun 9, 2023

Summary

This PR improves the performance of the comments extraction by skipping over bodies that contain no comments.

The visitor already avoided visiting the children of a node if there are no comments in its range but we aren't doing this yet for bodies because bodies are not nodes.
This PR adds the logic for skipping over bodies if there's no comment in the body range.

if test: # a comment 
	... 

The old implementation did visit the if statement, as well as the body and orelse because it contains a comment in its range. But this is unnecessary
because only the case header has a comment.

Test Plan

cargo test

@MichaReiser MichaReiser changed the title perf(formatter): Avoid skipping bodies without comments perf(formatter): Skip bodies without comments Jun 9, 2023
@MichaReiser MichaReiser added internal An internal refactor or improvement performance Potential performance improvement formatter Related to the formatter labels Jun 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.02      6.5±0.02ms     6.3 MB/sec    1.00      6.4±0.01ms     6.4 MB/sec
formatter/numpy/ctypeslib.py               1.01   1357.2±7.20µs    12.3 MB/sec    1.00   1339.5±2.51µs    12.4 MB/sec
formatter/numpy/globals.py                 1.01    131.5±0.23µs    22.4 MB/sec    1.00    129.9±0.17µs    22.7 MB/sec
formatter/pydantic/types.py                1.02      2.7±0.02ms     9.6 MB/sec    1.00      2.6±0.02ms     9.7 MB/sec
linter/all-rules/large/dataset.py          1.00     14.0±0.02ms     2.9 MB/sec    1.00     14.0±0.04ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.4±0.00ms     4.9 MB/sec    1.00      3.4±0.01ms     4.9 MB/sec
linter/all-rules/numpy/globals.py          1.00    423.2±0.61µs     7.0 MB/sec    1.00    423.3±0.60µs     7.0 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.9±0.01ms     4.3 MB/sec    1.00      5.9±0.01ms     4.3 MB/sec
linter/default-rules/large/dataset.py      1.00      6.7±0.01ms     6.0 MB/sec    1.01      6.8±0.01ms     6.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1471.2±3.38µs    11.3 MB/sec    1.01   1484.2±2.40µs    11.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    166.1±0.29µs    17.8 MB/sec    1.00    166.7±0.23µs    17.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.1±0.01ms     8.3 MB/sec    1.00      3.1±0.01ms     8.3 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.6±0.06ms     5.3 MB/sec    1.15      8.7±0.07ms     4.7 MB/sec
formatter/numpy/ctypeslib.py               1.00  1556.5±15.14µs    10.7 MB/sec    1.12  1739.9±23.82µs     9.6 MB/sec
formatter/numpy/globals.py                 1.00    149.3±3.21µs    19.8 MB/sec    1.11    165.5±4.76µs    17.8 MB/sec
formatter/pydantic/types.py                1.00      3.1±0.04ms     8.2 MB/sec    1.13      3.5±0.04ms     7.2 MB/sec
linter/all-rules/large/dataset.py          1.00     16.4±0.10ms     2.5 MB/sec    1.01     16.6±0.12ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.2±0.05ms     4.0 MB/sec    1.01      4.2±0.04ms     3.9 MB/sec
linter/all-rules/numpy/globals.py          1.00    497.1±5.72µs     5.9 MB/sec    1.00    499.3±6.78µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.0±0.05ms     3.6 MB/sec    1.01      7.1±0.06ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.00      8.2±0.04ms     5.0 MB/sec    1.00      8.2±0.06ms     4.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1755.9±29.79µs     9.5 MB/sec    1.00  1747.9±23.44µs     9.5 MB/sec
linter/default-rules/numpy/globals.py      1.00    198.1±2.83µs    14.9 MB/sec    1.01    200.1±6.00µs    14.7 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.7±0.03ms     6.8 MB/sec    1.00      3.7±0.03ms     6.9 MB/sec

@MichaReiser
Copy link
Member Author

The benchmarks compare against main which does not yet have the if statement formatting. But we can see that the increase goes down from 25% slowdown to an at most 18% slowdown.

@MichaReiser MichaReiser requested a review from konstin June 9, 2023 07:17
@konstin
Copy link
Member

konstin commented Jun 9, 2023

With #4964 (comment), i don't really see much performance difference

@MichaReiser MichaReiser force-pushed the fix-bin-oip-with-leading-comments branch 2 times, most recently from 7458b5b to e341c1c Compare June 9, 2023 08:56
Base automatically changed from fix-bin-oip-with-leading-comments to main June 9, 2023 09:02
Improve the performance of the comments extraction by skipping bodies
that contain no comments.
@MichaReiser
Copy link
Member Author

With #4964 (comment), i don't really see much performance difference

The linux result are in the range of what I expected and was able to reproduce locally. Not sure what happened with the windows run.

@MichaReiser MichaReiser merged commit 111e1f9 into main Jun 9, 2023
@MichaReiser MichaReiser deleted the perf-skip-body branch June 9, 2023 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter internal An internal refactor or improvement performance Potential performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants