Skip to content

Comments

Use single lookup for leading, dangling, and trailing comments#6589

Merged
MichaReiser merged 1 commit intomainfrom
comments-single-lookup
Aug 15, 2023
Merged

Use single lookup for leading, dangling, and trailing comments#6589
MichaReiser merged 1 commit intomainfrom
comments-single-lookup

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Aug 15, 2023 •
edited
Loading

Summary

This is a preparation PR for #6561. The fmt: skip support requires looking up the trailing comments to determine whether a node is suppressed or not. However, looking up the comments for a node isn't free and we would now perform 6 comment lookups for every statement:

  • The leading and trailing comments each once to test if the node falls into a suppressed range
  • Once to determine whether the statement is suppressed by a trailing comment.
  • Once to format the leading comments
  • Once to format the dangling comments
  • Once to format the trailing comments

This is a lot. This PR reduces the number of lookups to 2 (or 3 if the dangling comment formatting is handled by the node):

  • Once to test the range suppression comments
  • Once to test the trailing suppresseion comment and format the leading, dangling, and trailing comments.

Performance

This results in a small performance improvement

Gnuplot not found, using plotters backend
formatter/numpy/globals.py
                        time:   [38.282 µs 38.439 µs 38.579 µs]
                        thrpt:  [76.484 MiB/s 76.761 MiB/s 77.077 MiB/s]
                 change:
                        time:   [-2.6719% -2.2332% -1.8094%] (p = 0.00 < 0.05)
                        thrpt:  [+1.8427% +2.2842% +2.7452%]
                        Performance has improved.
formatter/pydantic/types.py
                        time:   [801.74 µs 802.72 µs 803.74 µs]
                        thrpt:  [31.731 MiB/s 31.771 MiB/s 31.810 MiB/s]
                 change:
                        time:   [-0.0133% +0.4071% +0.8294%] (p = 0.06 > 0.05)
                        thrpt:  [-0.8226% -0.4055% +0.0133%]
                        No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
  4 (4.00%) high mild
  2 (2.00%) high severe
formatter/numpy/ctypeslib.py
                        time:   [395.65 µs 396.25 µs 396.95 µs]
                        thrpt:  [41.948 MiB/s 42.021 MiB/s 42.086 MiB/s]
                 change:
                        time:   [-1.8160% -1.4214% -1.0641%] (p = 0.00 < 0.05)
                        thrpt:  [+1.0755% +1.4419% +1.8496%]
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe
formatter/large/dataset.py
                        time:   [2.0272 ms 2.0319 ms 2.0380 ms]
                        thrpt:  [19.962 MiB/s 20.022 MiB/s 20.069 MiB/s]
                 change:
                        time:   [+0.4893% +0.9936% +1.4942%] (p = 0.00 < 0.05)
                        thrpt:  [-1.4722% -0.9838% -0.4869%]
                        Change within noise threshold.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe

Test Plan

cargo test

@MichaReiser
Copy link
Member Author

MichaReiser commented Aug 15, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@MichaReiser MichaReiser force-pushed the comments-single-lookup branch 2 times, most recently from 29ba963 to 5811fb9 Compare August 15, 2023 05:58
@MichaReiser MichaReiser marked this pull request as ready for review August 15, 2023 05:58
@MichaReiser MichaReiser added the formatter Related to the formatter label Aug 15, 2023
@MichaReiser MichaReiser force-pushed the comments-single-lookup branch from 5811fb9 to be450a6 Compare August 15, 2023 06:01
@MichaReiser MichaReiser mentioned this pull request Aug 15, 2023
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Aug 15, 2023 •
edited
Loading

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      3.9±0.03ms    10.4 MB/sec    1.00      3.9±0.03ms    10.5 MB/sec
formatter/numpy/ctypeslib.py               1.02    780.5±2.85µs    21.3 MB/sec    1.00   767.9±34.81µs    21.7 MB/sec
formatter/numpy/globals.py                 1.04     78.9±0.37µs    37.4 MB/sec    1.00     76.2±1.47µs    38.7 MB/sec
formatter/pydantic/types.py                1.03  1574.7±28.52µs    16.2 MB/sec    1.00   1526.5±6.92µs    16.7 MB/sec
linter/all-rules/large/dataset.py          1.00     10.3±0.02ms     4.0 MB/sec    1.00     10.3±0.03ms     3.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.8±0.01ms     6.0 MB/sec    1.00      2.8±0.02ms     6.0 MB/sec
linter/all-rules/numpy/globals.py          1.01    396.9±0.46µs     7.4 MB/sec    1.00    393.4±0.59µs     7.5 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.4±0.04ms     4.8 MB/sec    1.00      5.4±0.02ms     4.8 MB/sec
linter/default-rules/large/dataset.py      1.01      5.5±0.02ms     7.4 MB/sec    1.00      5.5±0.02ms     7.5 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1217.6±1.38µs    13.7 MB/sec    1.00   1209.9±2.79µs    13.8 MB/sec
linter/default-rules/numpy/globals.py      1.00    141.5±0.33µs    20.9 MB/sec    1.00    141.0±0.30µs    20.9 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.5±0.01ms    10.3 MB/sec    1.00      2.5±0.01ms    10.3 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.02      4.2±0.01ms     9.8 MB/sec    1.00      4.1±0.02ms    10.0 MB/sec
formatter/numpy/ctypeslib.py               1.02    790.6±9.06µs    21.1 MB/sec    1.00   778.4±23.75µs    21.4 MB/sec
formatter/numpy/globals.py                 1.03     81.1±0.46µs    36.4 MB/sec    1.00     78.5±1.10µs    37.6 MB/sec
formatter/pydantic/types.py                1.03  1632.1±24.20µs    15.6 MB/sec    1.00  1590.9±17.52µs    16.0 MB/sec
linter/all-rules/large/dataset.py          1.00     12.6±0.05ms     3.2 MB/sec    1.00     12.6±0.07ms     3.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.6±0.02ms     4.7 MB/sec    1.00      3.5±0.01ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.01    370.5±5.06µs     8.0 MB/sec    1.00    368.5±8.38µs     8.0 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.6±0.02ms     3.9 MB/sec    1.00      6.5±0.02ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.01      7.0±0.04ms     5.8 MB/sec    1.00      6.9±0.03ms     5.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1452.3±5.78µs    11.5 MB/sec    1.00   1440.4±6.11µs    11.6 MB/sec
linter/default-rules/numpy/globals.py      1.00    150.9±1.23µs    19.6 MB/sec    1.00    150.8±1.98µs    19.6 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.1±0.02ms     8.2 MB/sec    1.00      3.1±0.03ms     8.2 MB/sec

@MichaReiser MichaReiser mentioned this pull request Aug 15, 2023
Merged
@MichaReiser MichaReiser force-pushed the comments-single-lookup branch from be450a6 to 072263c Compare August 15, 2023 14:20
@MichaReiser MichaReiser merged commit 29c0b9f into main Aug 15, 2023
@MichaReiser MichaReiser deleted the comments-single-lookup branch August 15, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

No one assigned

Labels

formatter Related to the formatter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MichaReiser @charliermarsh