Skip to content

Comments

perf(linter): no-fallthrough: Use string matching instead of Regex for default comment pattern#6008

Merged
graphite-app[bot] merged 1 commit intomainfrom
09-23-perf_linter_no-fallthrough_use_string_matching_instead_of_regex_for_default_comment_pattern
Sep 24, 2024
Merged

perf(linter): no-fallthrough: Use string matching instead of Regex for default comment pattern#6008
graphite-app[bot] merged 1 commit intomainfrom
09-23-perf_linter_no-fallthrough_use_string_matching_instead_of_regex_for_default_comment_pattern

Conversation

@camchenry
Copy link
Member

@camchenry camchenry commented Sep 23, 2024

Profiling has shown this rule to be a consistent slow point, and in particular, the Regex construction is slow.

Screenshot 2024-09-23 at 7 12 58 PM

This PR improves the situation in two ways:

  1. A Regex is only constructed when there is a custom comment pattern (which is likely the minority of cases)
  2. For the default comment pattern (when no custom pattern is passed), we now use a simple string matcher function, instead of a full-blown regex matcher. I believe this should be faster since it involves much less work, though can still allocate a String.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @camchenry and the rest of your teammates on Graphite Graphite

@graphite-app
Copy link
Contributor

graphite-app bot commented Sep 23, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the A-linter Area - Linter label Sep 23, 2024
@camchenry camchenry force-pushed the 09-23-perf_linter_no-fallthrough_use_string_matching_instead_of_regex_for_default_comment_pattern branch from abc0b33 to eb09ff1 Compare September 23, 2024 23:37
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 23, 2024

CodSpeed Performance Report

Merging #6008 will not alter performance

Comparing 09-23-perf_linter_no-fallthrough_use_string_matching_instead_of_regex_for_default_comment_pattern (5ae3f36) with main (5a0d17c)

Summary

✅ 29 untouched benchmarks

@camchenry camchenry marked this pull request as ready for review September 23, 2024 23:53
@DonIsaac DonIsaac added the 0-merge Merge with Graphite Merge Queue label Sep 24, 2024
@graphite-app
Copy link
Contributor

graphite-app bot commented Sep 24, 2024

Merge activity

  • Sep 23, 9:05 PM EDT: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Sep 23, 9:05 PM EDT: DonIsaac added this pull request to the Graphite merge queue.
  • Sep 23, 9:10 PM EDT: DonIsaac merged this pull request with the Graphite merge queue.

…for default comment pattern (#6008)

Profiling has shown this rule to be a consistent slow point, and in particular, the Regex construction is slow.

<img width="1323" alt="Screenshot 2024-09-23 at 7 12 58 PM" src="https://github.com/user-attachments/assets/1d9b367d-eeda-4b19-b0a3-463e54ac4334">

This PR improves the situation in two ways:

1. A `Regex` is only constructed when there is a custom comment pattern (which is likely the minority of cases)
2. For the default comment pattern (when no custom pattern is passed), we now use a simple string matcher function, instead of a full-blown regex matcher. I believe this should be faster since it involves much less work, though can still allocate a `String`.
@DonIsaac DonIsaac force-pushed the 09-23-perf_linter_no-fallthrough_use_string_matching_instead_of_regex_for_default_comment_pattern branch from eb09ff1 to 5ae3f36 Compare September 24, 2024 01:06
@graphite-app graphite-app bot merged commit 5ae3f36 into main Sep 24, 2024
@graphite-app graphite-app bot deleted the 09-23-perf_linter_no-fallthrough_use_string_matching_instead_of_regex_for_default_comment_pattern branch September 24, 2024 01:10
@oxc-bot oxc-bot mentioned this pull request Sep 24, 2024
Boshen added a commit that referenced this pull request Sep 24, 2024
## [0.9.8] - 2024-09-24

### Bug Fixes

- e3c8a12 linter: Fix panic in sort-keys (#6017) (Boshen)
- 4771492 linter: Fix `import/no_cycle` with `ignoreTypes` (#5995)
(Boshen)

### Performance

- 5ae3f36 linter: `no-fallthrough`: Use string matching instead of Regex
for default comment pattern (#6008) (camchenry)
- 2b17003 linter, prettier, diagnostics: Use `FxHashMap` instead of
`std::collections::HashMap` (#5993) (camchenry)

---------

Co-authored-by: Boshen <[email protected]>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue A-linter Area - Linter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants