How does ruff make a decision about performance impact?

[mayty]

Are there any quantifiable tests done before stating that a rule has a performance impact on code execution? Stating that some way is more efficient than the other doesn't feel like enough information to make an informed decision.

For example SIM118 states:

key in dict is more readable and efficient than key in dict.keys()

flake-simplify doesn't specify that it is more efficient than key in dict.keys(), while StackOverflow comment which is used as cause for the rule states the following:

In Python 3, ..., however in practice it would be slower by at least a constant value

Which is purely theoretical speculation, although having a valid basis.

To be clear, I am not trying to argue if this specific rule has or has not performance impact. I'm advocating for including a reason/proof of performance impact in the "Why is this bad?" section to allow an informed decision.

[ntBre]

I think the short answer here is "no" we don't have a test/demonstration requirement for these claims, at least in the rules I've seen.

I think it's a good question though. Even this case isn't so obvious. I didn't know, for example, that .keys() returns a set-like view object, so it's not just iterating over the keys like I expected.

One problem with including these tests is that a lot of these benchmarks will be context-dependent. Maybe that's an argument against using efficiency as a rule justification in general, but I think it's more like any performance-related rules come with the caveat that you should benchmark your specific use case.

On the other hand, if the efficiency claims are outright wrong, then it definitely shouldn't be mentioned in the docs. I don't think that's the case for SIM118, though, based on some tiny benchmarks I just ran and also looking at the CPython source code.

[mayty]

a lot of these benchmarks will be context-dependent.

That's my point, using phrasing like more efficient etc. can be misleading. Including benchmarks and understanding the scope could be of massive help in making decisions. Keeping benchmark trails could also help with making sure the statements are at least somewhat up to date, e.g. "We are dropping python2 support in our linter, maybe it's time to rerun benchmarks, that were executed only on python2".
Also "performance difference" can mean a lot of things. For example, a 0.00001% performance difference between implementations can be statistically significant while carrying no real-life difference for most applications. Or 1ns vs 2ns for rare operations can be totally fine, but "2x difference" sounds scary.
I know a lot of people who would think "Linter devs have written that it has a performance difference, then there is a performance difference".

any performance-related rules come with the caveat that you should benchmark your specific use case.

Yes, but I assume lots of people won't because "A lot of smart people at @{linter_name} have probably thoroughly benchmarked it multiple times on all existing platforms before including it as a rule, what is there more to benchmark".

On the other hand, if the efficiency claims are outright wrong, then it definitely shouldn't be mentioned in the docs

And for that reason, some benchmarks, however simple they might be, must be run. And if they are run, it should be documented somewhere

[mayty]

On a different note. All PERF* rules have the following note:

Note that, as with all perflint rules, this is only intended as a micro-optimization, and will have a negligible impact on performance in most cases.

While rules that claim some approach is more efficient, do not. It is therefore valid to assume these rules will have a non-negligible impact on performance in most cases.

[mayty]

The best-case solution, in my mind, would be the ability for people to submit their minimal repeatable benchmarks along with their results, so other people could easily find them

[ntBre]

Fair enough 🙂

I think I agree with you in a general sense about benchmarks, and people are certainly welcome to submit issues or PRs suggesting or adding these benchmarks. I'm just picturing reviewing these PRs, and I think I would tend toward removing any mention of performance over trying to assess the quality or applicability of a submitted benchmark. It just seems like a high maintenance burden and not really relevant to most rules. For example, SIM118 would be fine if it just said "more readable" and left it there. But I'm interested to hear other thoughts too.

[mayty]

How about the following approach:

1. Remove performance mentions from the rule pages
2. Add new GitHub discussions category "rule benchmarks" with naming convention [@{rule_id}] @{human_description}, for example [SIM118] python3.12 on x86_64 system
3. Add the section "benchmarks" to all linter rules with a disclaimer like "User-generated content, caution advised"
   • with a link like https://github.com/astral-sh/ruff/discussions/categories/general?discussions_q=impact+in%3Atitle (with [@{rule_id}] as title search text), so the discussions with benchmarked could be easily accessed from the rule page
   • Or with widgets/links leading to discussions found on the rule page load with a request similar to the aforementioned (I think there is a GraphQL API for GitHub discussions)

This way people could freely submit their benchmarks to any rules and discuss them. And possible labels like "validated" and "unvalidated", with "unvalidated" being the default and "valid" given out by maintainers, if they have time to validate them at some point, could further entice people to think before making any decisions based on these discussions (assuming most discussions would have a scary red label "unvalidated")