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

Use Matchit to Resolve Per-File Settings #11111

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

ibraheemdev
Copy link
Member

Summary

Continuation of #9444.

When the formatter is fully cached, it turns out we actually spend meaningful time mapping from file to Settings (since we use a hierarchical approach to settings). Using matchit rather than BTreeMap improves fully-cached performance by anywhere from 2-5% depending on the project, and since these are all implementation details of Resolver, it's minimally invasive.

matchit supports escaping routing characters so this change should now be fully compatible.

Test Plan

On my machine I'm seeing a ~3% improvement with this change.

hyperfine --warmup 20 -i "./target/release/main format ../airflow" "./target/release/ruff format ../airflow"
Benchmark 1: ./target/release/main format ../airflow
  Time (mean ± σ):      58.1 ms ±   1.4 ms    [User: 63.1 ms, System: 66.5 ms]
  Range (min … max):    56.1 ms …  62.9 ms    49 runs
 
Benchmark 2: ./target/release/ruff format ../airflow
  Time (mean ± σ):      56.6 ms ±   1.5 ms    [User: 57.8 ms, System: 67.7 ms]
  Range (min … max):    54.1 ms …  63.0 ms    51 runs
 
Summary
  ./target/release/ruff format ../airflow ran
    1.03 ± 0.04 times faster than ./target/release/main format ../airflow

Copy link
Contributor

github-actions bot commented Apr 23, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Nice!!!

{
Ok(_) => {}
Err(InsertError::Conflict { .. }) => {}
Err(_) => unreachable!("file paths are escaped before being inserted in the router"),
Copy link
Member

Choose a reason for hiding this comment

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

I was curious about this, and I think I convinced myself that this is correct because all other possible errors (aside from Conflict) can only occur as a result of characters that the router treats as special. And the escaping done above means that there should be precisely zero special characters in the path.

(It might make sense to add an escape routine to matchit. Similar to regex::escape. That way, you can provide more of a stronger guarantee here.)

Copy link
Member Author

@ibraheemdev ibraheemdev Apr 23, 2024

Choose a reason for hiding this comment

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

Yeah after escaping we guarantee that we're inserting a static route, so none of the other errors are possible (barring a bug in matchit). An escape function in matchit is a good idea. I was also considering optimizing the double String::replace but decided it wasn't worth it for us.

self.settings.push(settings);

// normalize the path to use `/` separators and escape the '{' and '}' characters,
// which matchit uses for routing parameters
Copy link
Member

Choose a reason for hiding this comment

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

Are there any other characters that we need to watch out for here? In my previous PR I mentioned colons, which seems irrelevant, but e.g. {*foo} is a special routing parameter in matchit, right? I assume that's also irrelevant as long as we're escaping the { and } characters.

Copy link
Member Author

@ibraheemdev ibraheemdev Apr 23, 2024

Choose a reason for hiding this comment

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

* is only considered after a non-escaped {, so that shouldn't be a problem.

@charliermarsh charliermarsh added the performance Potential performance improvement label Apr 23, 2024
@ibraheemdev ibraheemdev merged commit 1c8849f into astral-sh:main Apr 24, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Potential performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants