Refactor `noqa` directive parsing away from regex-based implementation #5554

charliermarsh · 2023-07-06T05:45:06Z

Summary

This PR removes our regex-based implementation in favor of "manual" parsing.

I tried a couple different implementations. In the benchmarks below:

Directive/Regex is our implementation on main.
Directive/Find just uses text.find("noqa"), which is insufficient, since it doesn't cover case-insensitive variants like NOQA, and doesn't handle multiple noqa matches in a single like, like # Here's a noqa comment # noqa: F401. But it's kind of a baseline.
Directive/Memchr uses three memchr iterative finders (one for noqa, NOQA, and NoQA).
Directive/AhoCorasick is roughly the variant checked-in here.

The raw results:

Directive/Regex/# noqa: F401
                        time:   [273.69 ns 274.71 ns 276.03 ns]
                        change: [+1.4467% +1.8979% +2.4243%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 15 outliers among 100 measurements (15.00%)
  3 (3.00%) low mild
  8 (8.00%) high mild
  4 (4.00%) high severe
Directive/Find/# noqa: F401
                        time:   [66.972 ns 67.048 ns 67.132 ns]
                        change: [+2.8292% +2.9377% +3.0540%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 15 outliers among 100 measurements (15.00%)
  1 (1.00%) low severe
  3 (3.00%) low mild
  8 (8.00%) high mild
  3 (3.00%) high severe
Directive/AhoCorasick/# noqa: F401
                        time:   [76.922 ns 77.189 ns 77.536 ns]
                        change: [+0.4265% +0.6862% +0.9871%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
  1 (1.00%) low mild
  3 (3.00%) high mild
  4 (4.00%) high severe
Directive/Memchr/# noqa: F401
                        time:   [62.627 ns 62.654 ns 62.679 ns]
                        change: [-0.1780% -0.0887% -0.0120%] (p = 0.03 < 0.05)
                        Change within noise threshold.
Found 11 outliers among 100 measurements (11.00%)
  1 (1.00%) low severe
  5 (5.00%) low mild
  3 (3.00%) high mild
  2 (2.00%) high severe
Directive/Regex/# noqa: F401, F841
                        time:   [321.83 ns 322.39 ns 322.93 ns]
                        change: [+8602.4% +8623.5% +8644.5%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) low severe
  2 (2.00%) low mild
  1 (1.00%) high mild
  1 (1.00%) high severe
Directive/Find/# noqa: F401, F841
                        time:   [78.618 ns 78.758 ns 78.896 ns]
                        change: [+1.6909% +1.8771% +2.0628%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild
Directive/AhoCorasick/# noqa: F401, F841
                        time:   [87.739 ns 88.057 ns 88.468 ns]
                        change: [+0.1843% +0.4685% +0.7854%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 11 outliers among 100 measurements (11.00%)
  5 (5.00%) low mild
  3 (3.00%) high mild
  3 (3.00%) high severe
Directive/Memchr/# noqa: F401, F841
                        time:   [80.674 ns 80.774 ns 80.860 ns]
                        change: [-0.7343% -0.5633% -0.4031%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 14 outliers among 100 measurements (14.00%)
  4 (4.00%) low severe
  9 (9.00%) low mild
  1 (1.00%) high mild
Directive/Regex/# noqa  time:   [194.86 ns 195.93 ns 196.97 ns]
                        change: [+11973% +12039% +12103%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
  5 (5.00%) low mild
  1 (1.00%) high mild
Directive/Find/# noqa   time:   [25.327 ns 25.354 ns 25.383 ns]
                        change: [+3.8524% +4.0267% +4.1845%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
  6 (6.00%) high mild
  3 (3.00%) high severe
Directive/AhoCorasick/# noqa
                        time:   [34.267 ns 34.368 ns 34.481 ns]
                        change: [+0.5646% +0.8505% +1.1281%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
  5 (5.00%) high mild
Directive/Memchr/# noqa time:   [21.770 ns 21.818 ns 21.874 ns]
                        change: [-0.0990% +0.1464% +0.4046%] (p = 0.26 > 0.05)
                        No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
  4 (4.00%) low mild
  4 (4.00%) high mild
  2 (2.00%) high severe
Directive/Regex/# type: ignore # noqa: E501
                        time:   [278.76 ns 279.69 ns 280.72 ns]
                        change: [+7449.4% +7469.8% +7490.5%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild
  1 (1.00%) high severe
Directive/Find/# type: ignore # noqa: E501
                        time:   [67.791 ns 67.976 ns 68.184 ns]
                        change: [+2.8321% +3.1735% +3.5418%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
  5 (5.00%) high mild
  1 (1.00%) high severe
Directive/AhoCorasick/# type: ignore # noqa: E501
                        time:   [75.908 ns 76.055 ns 76.210 ns]
                        change: [+0.9269% +1.1427% +1.3955%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high severe
Directive/Memchr/# type: ignore # noqa: E501
                        time:   [72.549 ns 72.723 ns 72.957 ns]
                        change: [+1.5881% +1.9660% +2.3974%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 15 outliers among 100 measurements (15.00%)
  10 (10.00%) high mild
  5 (5.00%) high severe
Directive/Regex/# type: ignore # nosec
                        time:   [66.967 ns 67.075 ns 67.207 ns]
                        change: [+1713.0% +1715.8% +1718.9%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
  1 (1.00%) low severe
  3 (3.00%) low mild
  2 (2.00%) high mild
  4 (4.00%) high severe
Directive/Find/# type: ignore # nosec
                        time:   [18.505 ns 18.548 ns 18.597 ns]
                        change: [+1.3520% +1.6976% +2.0333%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
  4 (4.00%) high mild
Directive/AhoCorasick/# type: ignore # nosec
                        time:   [16.162 ns 16.206 ns 16.252 ns]
                        change: [+1.2919% +1.5587% +1.8430%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe
Directive/Memchr/# type: ignore # nosec
                        time:   [39.192 ns 39.233 ns 39.276 ns]
                        change: [+0.5164% +0.7456% +0.9790%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 13 outliers among 100 measurements (13.00%)
  2 (2.00%) low severe
  4 (4.00%) low mild
  3 (3.00%) high mild
  4 (4.00%) high severe
Directive/Regex/# some very long comment that # is interspersed with characters but # no directive
                        time:   [81.460 ns 81.578 ns 81.703 ns]
                        change: [+2093.3% +2098.8% +2104.2%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) low mild
  2 (2.00%) high mild
Directive/Find/# some very long comment that # is interspersed with characters but # no directive
                        time:   [26.284 ns 26.331 ns 26.387 ns]
                        change: [+0.7554% +1.1027% +1.3832%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
  5 (5.00%) high mild
  1 (1.00%) high severe
Directive/AhoCorasick/# some very long comment that # is interspersed with characters but # no direc...
                        time:   [28.643 ns 28.714 ns 28.787 ns]
                        change: [+1.3774% +1.6780% +2.0028%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild
Directive/Memchr/# some very long comment that # is interspersed with characters but # no directive
                        time:   [55.766 ns 55.831 ns 55.897 ns]
                        change: [+1.5802% +1.7476% +1.9021%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) low mild

While memchr is faster than aho-corasick in some of the common cases (like # noqa: F401), the latter is way, way faster when there isn't a match (like 2x faster -- see the last two cases). Since most comments aren't noqa comments, this felt like the right tradeoff. Note that all implementations are significantly faster than the regex version.

(I know I originally reported a 10x speedup, but I ended up improving the regex version a bit in some prior PRs, so it got unintentionally faster via some refactors.)

There's also one behavior change in here, which is that we now allow variable spaces, e.g., #noqa or # noqa. Previously, we required exactly one space. This thus closes #5177.

charliermarsh · 2023-07-06T05:48:55Z

(The speed-up here is between 3x and 10x depending on the case.)

github-actions · 2023-07-06T06:25:46Z

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+7, -0, 0 error(s))

airflow (+6, -0)

+ airflow/kubernetes/pod_launcher.py:25:56: F401 [*] `airflow.kubernetes.pod_launcher_deprecated.PodLauncher` imported but unused
+ airflow/kubernetes/pod_launcher.py:25:69: F401 [*] `airflow.kubernetes.pod_launcher_deprecated.PodStatus` imported but unused
+ airflow/kubernetes/volume.py:27:69: F401 [*] `airflow.providers.cncf.kubernetes.backcompat.volume.Volume` imported but unused
+ airflow/kubernetes/volume_mount.py:27:75: F401 [*] `airflow.providers.cncf.kubernetes.backcompat.volume_mount.VolumeMount` imported but unused
+ dev/breeze/src/airflow_breeze/commands/release_candidate_command.py:205:111: E501 Line too long (244 > 110 characters)
+ dev/breeze/src/airflow_breeze/commands/release_candidate_command.py:205:232: COM812 [*] Trailing comma missing

setuptools (+1, -0)

+ setuptools/tests/test_logging.py:23:12: F401 [*] `setuptools` imported but unused

Rules changed: 3

Rule	Changes	Additions
F401	5	5
E501	1	1
COM812	1	1

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.8±0.02ms     5.2 MB/sec    1.00      7.8±0.01ms     5.2 MB/sec
formatter/numpy/ctypeslib.py               1.00   1747.0±3.95µs     9.5 MB/sec    1.00   1741.5±2.20µs     9.6 MB/sec
formatter/numpy/globals.py                 1.00    195.5±0.28µs    15.1 MB/sec    1.00    196.0±0.20µs    15.1 MB/sec
formatter/pydantic/types.py                1.01      3.8±0.01ms     6.7 MB/sec    1.00      3.8±0.03ms     6.7 MB/sec
linter/all-rules/large/dataset.py          1.00     13.6±0.06ms     3.0 MB/sec    1.00     13.6±0.07ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.4±0.01ms     4.8 MB/sec    1.00      3.4±0.01ms     4.9 MB/sec
linter/all-rules/numpy/globals.py          1.01    439.7±0.56µs     6.7 MB/sec    1.00    435.7±0.78µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.0±0.01ms     4.2 MB/sec    1.00      6.0±0.01ms     4.3 MB/sec
linter/default-rules/large/dataset.py      1.02      7.0±0.02ms     5.8 MB/sec    1.00      6.9±0.02ms     5.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1518.0±2.53µs    11.0 MB/sec    1.00   1497.2±2.71µs    11.1 MB/sec
linter/default-rules/numpy/globals.py      1.01    172.5±0.24µs    17.1 MB/sec    1.00    170.2±0.35µs    17.3 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.1±0.01ms     8.2 MB/sec    1.00      3.1±0.01ms     8.2 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      9.6±0.28ms     4.2 MB/sec    1.02      9.8±0.33ms     4.2 MB/sec
formatter/numpy/ctypeslib.py               1.01      2.1±0.06ms     8.0 MB/sec    1.00      2.1±0.05ms     8.0 MB/sec
formatter/numpy/globals.py                 1.01   239.9±11.55µs    12.3 MB/sec    1.00    237.3±6.68µs    12.4 MB/sec
formatter/pydantic/types.py                1.03      4.7±0.20ms     5.4 MB/sec    1.00      4.6±0.13ms     5.5 MB/sec
linter/all-rules/large/dataset.py          1.03     17.0±0.55ms     2.4 MB/sec    1.00     16.5±0.56ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.05      4.5±0.22ms     3.7 MB/sec    1.00      4.3±0.19ms     3.9 MB/sec
linter/all-rules/numpy/globals.py          1.00   522.4±20.84µs     5.6 MB/sec    1.00   520.4±17.75µs     5.7 MB/sec
linter/all-rules/pydantic/types.py         1.01      7.5±0.35ms     3.4 MB/sec    1.00      7.4±0.31ms     3.4 MB/sec
linter/default-rules/large/dataset.py      1.01      8.5±0.25ms     4.8 MB/sec    1.00      8.4±0.31ms     4.8 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01  1794.4±56.50µs     9.3 MB/sec    1.00  1778.8±68.27µs     9.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    203.3±6.43µs    14.5 MB/sec    1.00    204.0±4.22µs    14.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.8±0.18ms     6.6 MB/sec    1.00      3.9±0.12ms     6.6 MB/sec

crates/ruff/src/noqa.rs

konstin · 2023-07-06T08:41:05Z

crates/ruff/src/noqa.rs

+                codes_start += text[codes_start..]
+                    .find(|c: char| !c.is_whitespace())
+                    .unwrap_or(0);


How does this behave with # noqa: ?

crates/ruff/src/noqa.rs

konstin · 2023-07-06T08:43:24Z

that's a neat improvement!

crates/ruff/src/noqa.rs

charliermarsh requested a review from MichaReiser July 6, 2023 05:45

charliermarsh added performance Potential performance improvement suppression Related to supression of violations e.g. noqa labels Jul 6, 2023

charliermarsh force-pushed the charlie/noqa-parser branch from 28d0adf to 77ed42a Compare July 6, 2023 05:49

konstin reviewed Jul 6, 2023

View reviewed changes

MichaReiser approved these changes Jul 6, 2023

View reviewed changes

crates/ruff/src/noqa.rs Outdated Show resolved Hide resolved

crates/ruff/src/noqa.rs Outdated Show resolved Hide resolved

crates/ruff/src/noqa.rs Outdated Show resolved Hide resolved

charliermarsh added 6 commits July 6, 2023 11:15

Manual parser

61ff6b5

Add cargo benches

de0e0ea

Improve impl

29805a2

Check-in benchmarks

77ec4da

Lock-in implementation

219f058

Try to rewrite parsed-file-exemption

0894dba

charliermarsh force-pushed the charlie/noqa-parser branch 3 times, most recently from 176be06 to cdb295d Compare July 6, 2023 15:47

Revert

dbc30ae

charliermarsh force-pushed the charlie/noqa-parser branch from cdb295d to dbc30ae Compare July 6, 2023 15:53

charliermarsh enabled auto-merge (squash) July 6, 2023 15:58

charliermarsh merged commit cc82208 into main Jul 6, 2023

charliermarsh deleted the charlie/noqa-parser branch July 6, 2023 16:03

p-linnane mentioned this pull request Jul 12, 2023

ruff 0.0.278 Homebrew/homebrew-core#136446

Merged

cnpryer mentioned this pull request Aug 30, 2023

Exclude pragma comments from measured line width #6772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `noqa` directive parsing away from regex-based implementation #5554

Refactor `noqa` directive parsing away from regex-based implementation #5554

charliermarsh commented Jul 6, 2023 •

edited

Loading

charliermarsh commented Jul 6, 2023

github-actions bot commented Jul 6, 2023 •

edited

Loading

konstin Jul 6, 2023

konstin commented Jul 6, 2023

Refactor noqa directive parsing away from regex-based implementation #5554

Refactor noqa directive parsing away from regex-based implementation #5554

Conversation

charliermarsh commented Jul 6, 2023 • edited Loading

Summary

charliermarsh commented Jul 6, 2023

github-actions bot commented Jul 6, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

konstin Jul 6, 2023

Choose a reason for hiding this comment

konstin commented Jul 6, 2023

Refactor `noqa` directive parsing away from regex-based implementation #5554

Refactor `noqa` directive parsing away from regex-based implementation #5554

charliermarsh commented Jul 6, 2023 •

edited

Loading

github-actions bot commented Jul 6, 2023 •

edited

Loading