Fix `MissingDoc` quadratic behaviour #98153

nnethercote · 2022-06-16T00:39:13Z

Best reviewed one commit at a time.

The `MissingDoc` lint has quadratic behaviour when processing doc comments. This is a problem for large doc comments (e.g. 1000+ lines) when `deny(missing_code)` is enabled. A 1000-line doc comment using `//!` comments is represented as 1000 attributes on an item. The lint machinery iterates over each attribute with `visit_attribute`. `MissingDoc`'s impl of that function calls `with_lint_attrs`, which calls `enter_attrs`, which iterates over all 1000 attributes looking for a `doc(hidden)` attribute. I.e. for every attribute we iterate over all the other attributes. The fix is simple: don't call `with_lint_attrs` on attributes. This makes sense: `with_lint_attrs` is intended to iterate over the attributes on a language fragment like a statement or expression, but it doesn't need to be called on attributes themselves.

They each have a single call site.

nnethercote · 2022-06-16T00:41:59Z

Here are some benchmarks that are outside of rustc-perf that this affects. Here structopt-0.3.26-exaggerated is the same as structopt-0.3.26 but with one 1100 line doc comment tripled in size to 3300 lines to demonstrate the quadraticness.

Benchmark	Profile	Scenario	% Change	Significance Factor?
structopt-0.3.26-exaggerated	check	full	-36.04%	180.18x
structopt-0.3.26	check	full	-6.74%	33.68x
mockall-0.11.1	check	full	-3.34%	16.68x
crossbeam-0.8.1	check	full	-1.03%	5.16x
log-0.4.16	check	full	-0.69%	3.47x

nnethercote · 2022-06-16T05:20:18Z

I'm not expecting this to improve any of the benchmarks in rustc-perf, but let's double check:

@bors try @rust-timer queue

rust-timer · 2022-06-16T05:20:19Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-06-16T05:20:26Z

⌛ Trying commit be45f10 with merge 18823ac9b9380a53c4930484df17f531fc409671...

bors · 2022-06-16T07:23:13Z

☀️ Try build successful - checks-actions
Build commit: 18823ac9b9380a53c4930484df17f531fc409671 (18823ac9b9380a53c4930484df17f531fc409671)

rust-timer · 2022-06-16T07:23:14Z

Queued 18823ac9b9380a53c4930484df17f531fc409671 with parent 1b9daa6, future comparison URL.

rust-timer · 2022-06-16T08:41:04Z

Finished benchmarking commit (18823ac9b9380a53c4930484df17f531fc409671): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-0.3%	-0.4%	11
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-0.3%	-0.4%	11

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.9%	2.9%	1
Improvements 🎉 (primary)	-0.9%	-0.9%	1
Improvements 🎉 (secondary)	-4.4%	-4.9%	2
All 😿🎉 (primary)	-0.9%	-0.9%	1

Cycles

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.9%	-2.9%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-2.9%	-2.9%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

cjgillot · 2022-06-16T11:36:33Z

compiler/rustc_lint/src/late.rs

-            lint_callback!(cx, check_attribute, attr);
-        })
+    fn visit_attribute(&mut self, attr: &'tcx ast::Attribute) {
+        lint_callback!(self, check_attribute, attr);


What happens if we have an allow(some lint about attributes) followed by an attribute that triggers that lint?
Can we make sure we have the same behaviour whatever the position of those attributes (including expression, patterns...)?

Can you give a more specific example? I know this change fixes the performance problem and passes all the tests, but I don't know enough about linting to know if there is some edge case that it will miss.

Only clippy uses late lints on attributes. They should probably made early lints, and drop support for late lints on attributes.

For instance:

#[warn(macro_use_imports)] #[macro_use] extern crate std;

and with swapped attributes.

Is attribute order supposed to be significant? Are attributes supposed to apply to other attributes on the same item, or do they just apply to the item? The latter makes more sense to me, but I honestly don't know.

They should probably made early lints, and drop support for late lints on attributes.

Sorry, I don't understand this. Is this an argument in favour of this PR's change, or against it, or is it orthogonal?

I tried your example, with this code (including slight necessary changes):

#[warn(clippy::macro_use_imports)] #[macro_use] extern crate std as other_std; fn main() { }

With a standard rustc (not containing this PR's changes) I get much the same output from cargo clippy regardless of the order of the attributes:

warning: unused `#[macro_use]` import --> src/main.rs:2:1 | 2 | #[macro_use] | ^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default error: useless lint attribute --> src/main.rs:1:1 | 1 | #[warn(clippy::macro_use_imports)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if you just forgot a `!`, use: `#![warn(clippy::macro_use_imports)]` | = note: `#[deny(clippy::useless_attribute)]` on by default = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_attribute

If I add the ! as suggested then the error goes away, unsurprisingly.

Is attribute order supposed to be significant? Are attributes supposed to apply to other attributes on the same item, or do they just apply to the item? The latter makes more sense to me, but I honestly don't know.

For now, the order of attributes is not significant. My question is mostly: are we changing behaviour?

They should probably made early lints, and drop support for late lints on attributes.

Sorry, I don't understand this. Is this an argument in favour of this PR's change, or against it, or is it orthogonal?

This is rather orthogonal. I'll post a follow-up issue if you don't mind.

I tried your example, with this code (including slight necessary changes):

#[warn(clippy::macro_use_imports)] #[macro_use] extern crate std as other_std; fn main() { }

With a standard rustc (not containing this PR's changes) I get much the same output from cargo clippy regardless of the order of the attributes:

warning: unused `#[macro_use]` import --> src/main.rs:2:1 | 2 | #[macro_use] | ^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default error: useless lint attribute --> src/main.rs:1:1 | 1 | #[warn(clippy::macro_use_imports)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if you just forgot a `!`, use: `#![warn(clippy::macro_use_imports)]` | = note: `#[deny(clippy::useless_attribute)]` on by default = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_attribute

If I add the ! as suggested then the error goes away, unsurprisingly.

So, IIUC, the behaviour does not change. Great! Let's go ahead with this PR.

Excellent! Do you want me to do anything else, or is it ready for an r+?

cjgillot · 2022-06-18T08:48:58Z

@bors r+

bors · 2022-06-18T08:48:59Z

📌 Commit be45f10 has been approved by cjgillot

bors · 2022-06-18T09:57:03Z

⌛ Testing commit be45f10 with merge cdcc53b...

bors · 2022-06-18T12:22:58Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing cdcc53b to master...

rust-timer · 2022-06-18T13:52:27Z

Finished benchmarking commit (cdcc53b): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-0.3%	-0.4%	14
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-0.3%	-0.4%	14

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-1.9%	-1.9%	1
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.5%	2.5%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-4.4%	-4.4%	1
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote added 3 commits June 16, 2022 09:51

Remove unused hir_id arg from visit_attribute.

c9e9725

Inline and remove {enter,exit}_attrs functions.

be45f10

They each have a single call site.

rust-highfive assigned cjgillot Jun 16, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 16, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 16, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 16, 2022

cjgillot reviewed Jun 16, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 18, 2022

bors merged commit cdcc53b into rust-lang:master Jun 18, 2022

rustbot added this to the 1.63.0 milestone Jun 18, 2022

nnethercote deleted the fix-MissingDoc-quadratic-behaviour branch June 19, 2022 05:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `MissingDoc` quadratic behaviour #98153

Fix `MissingDoc` quadratic behaviour #98153

nnethercote commented Jun 16, 2022

nnethercote commented Jun 16, 2022

nnethercote commented Jun 16, 2022

rust-timer commented Jun 16, 2022

bors commented Jun 16, 2022

bors commented Jun 16, 2022

rust-timer commented Jun 16, 2022

rust-timer commented Jun 16, 2022

cjgillot Jun 16, 2022

nnethercote Jun 16, 2022

cjgillot Jun 16, 2022

nnethercote Jun 17, 2022

cjgillot Jun 17, 2022

nnethercote Jun 17, 2022

cjgillot commented Jun 18, 2022

bors commented Jun 18, 2022

bors commented Jun 18, 2022

bors commented Jun 18, 2022

rust-timer commented Jun 18, 2022

Fix MissingDoc quadratic behaviour #98153

Fix MissingDoc quadratic behaviour #98153

Conversation

nnethercote commented Jun 16, 2022

nnethercote commented Jun 16, 2022

nnethercote commented Jun 16, 2022

rust-timer commented Jun 16, 2022

bors commented Jun 16, 2022

bors commented Jun 16, 2022

rust-timer commented Jun 16, 2022

rust-timer commented Jun 16, 2022

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Jun 18, 2022

bors commented Jun 18, 2022

bors commented Jun 18, 2022

bors commented Jun 18, 2022

rust-timer commented Jun 18, 2022

Footnotes

Fix `MissingDoc` quadratic behaviour #98153

Fix `MissingDoc` quadratic behaviour #98153