rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.54k stars 12.73k forks source link

Unnecessary encoding of `def_span` query in metadata #81476

Open oli-obk opened 3 years ago

oli-obk commented 3 years ago

There were some perf regressions due #80191 can be seen here

Originally posted by @rylev in https://github.com/rust-lang/rust/issues/80919#issuecomment-767854932

We need to find out which kind (as what variant of def_kind) of things now get their def_span encoded and stop doing that.

Bonus points for also checking which additional DefKinds we can exclude to minimize the amount of spans that are put into metadata (someone remind me why we need spans in metadata at all?)

Aaron1011 commented 3 years ago

someone remind me why we need spans in metadata at all?

I believe the main use is for diagnostics. For macro_rules! macros, we need to preserve spans of tokens for correctness (this is especially important when a macro_rules! macro is generated by another macro).

oli-obk commented 3 years ago

ok... so DefKind::Macro(_) may be the only real user of this, though errors pointing into locally built crates may be improved by it.

I guess let's try to get the previous status quo reestablished first, not emitting Spans for things that don't need them.

cjgillot commented 3 years ago

someone remind me why we need spans in metadata at all?

I am wondering the same thing for ident_span. Those are used for one diagnostic, and to create the Ident for imported items. It's not clear to me whether the latter uses are semantic or diagnostic.

jyn514 commented 3 years ago

Rustdoc also uses def_span: https://github.com/rust-lang/rust/blob/b122908617436af187252572ed5db96850551380/src/librustdoc/clean/types.rs#L147-L148

tmiasko commented 3 years ago

In contrast, there is a report of a missing def_span in #81540 (using rustc before changes in question landed).

oli-obk commented 3 years ago

Hmm... so this perf regression may in fact be due to a bugfix