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.

Sign up for GitHub

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

perf: RuleTable::any_enabled #10971

Merged
merged 1 commit into from
Apr 16, 2024
Merged

perf: RuleTable::any_enabled #10971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions crates/ruff_linter/src/registry/rule_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ impl RuleSet {
/// assert!(set.contains(Rule::AmbiguousFunctionName));
/// assert!(!set.contains(Rule::BreakOutsideLoop));
/// ```
#[inline]
pub const fn contains(&self, rule: Rule) -> bool {
let rule = rule as u16;
let index = rule as usize / Self::SLICE_BITS as usize;
Expand All @@ -243,6 +244,20 @@ impl RuleSet {
self.0[index] & mask != 0
}

/// Returns `true` if any of the rules in `rules` are in this set.
#[inline]
pub const fn any(&self, rules: &[Rule]) -> bool {
let mut any = false;
let mut i = 0;

while i < rules.len() {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this entire function could just be written as rules.iter().any(|r| self.contains(r))? Did you try that and it was slower? (I believe it also has the benefit of short circuiting, which may or may not help depending on the typical length of rules.)

Copy link
Member Author

@MichaReiser MichaReiser Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The typical length is like 1-8 rules (where 8 is rare). The downside is that the function can't be const anymore ;).

But the performance is about the same. So lets use any as it is easier to understand.

Edit: Codspeed disagrees. The shift version is slightly faster (23% speedup instead of 20%)

Copy link
Member

Choose a reason for hiding this comment

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

Ah I missed the const requirement.

any |= self.contains(rules[i]);
Copy link
Member Author

Choose a reason for hiding this comment

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

I intentionally use an bitwise OR here to avoid any branching.

Copy link
Member

Choose a reason for hiding this comment

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

I'd bet it would compile down to the same code if you used an if. :-) But I actually find this pretty readable as it is personally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Acutally, it does not

The iter version has a jump

mov     rax, qword ptr [rsp - 112]
movzx   edx, byte ptr [rsp - 116]
mov     cl, 1
bt      rax, rdx
jb      .LBB0_2
movzx   ecx, byte ptr [rsp - 114]
bt      rax, rcx
setb    cl

The loop version does not (but it requires more instructions)

mov     byte ptr [rsp - 117], cl
lea     rax, [rsp - 117]
movzx   ecx, byte ptr [rsp - 114]
mov     edx, 1
shl     edx, cl
movzx   ecx, byte ptr [rsp - 116]
bts     edx, ecx
test    dword ptr [rsp - 112], edx
setne   byte ptr [rsp - 117]

https://godbolt.org/z/oWWzqc6s1

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I just meant, if self.contains(rules[i]) { any = true }.

It makes sense that the iter version has an extra jump because of the short circuit.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yeah, that probably compiles down to the same

i += 1;
}

any
}

/// Returns an iterator over the rules in this set.
///
/// ## Examples
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/settings/rule_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ impl RuleTable {
/// Returns whether any of the given rules should be checked.
#[inline]
pub const fn any_enabled(&self, rules: &[Rule]) -> bool {
self.enabled.intersects(&RuleSet::from_rules(rules))
self.enabled.any(rules)
}

/// Returns whether violations of the given rule should be fixed.
Expand Down
Loading