Diagnostics are slow when editing use items

[jonas-schievink]

When applying an edit like this to hir_def/src/lib.rs, diagnostics only show up after several seconds:

-use adt::VariantData;
+use adt::{VariantData;

 2057ms - handle_code_action
     2050ms - diagnostics
         2050ms - Module::diagnostics @ "<unknown>"
                0ms - crate_def_map:wait (4 calls)
                0ms - validate_body (4 calls)
                0ms - validate_module_item (4 calls)
             2050ms - ???

We seem to be missing a profiling call here. I suspect that the time is either used by displaying subsequent parse or resolve errors.

[flodiebold]

In my experience, unclosed use items like this create a lot of syntax errors in the file, so that may have something to do with this.

(Btw, automatically wrapping the item in {} like you implemented for expression might help with that?)

[stanciuadrian]

In this particular case there are 1500+ errors:

 2982ms - handle_code_action
     2970ms - diagnostics
         2969ms - Module::diagnostics @ "<unknown>"
             2969ms - DefMap::add_diagnostics
                    2ms - DefDiagnostic::add_to

                    [ ... 1490 calls omitted ... ]

                    2ms - DefDiagnostic::add_to
                    0ms - DefDiagnostic::add_to (206 calls)
                    0ms - ???
                0ms - crate_def_map:wait (4 calls)
                0ms - ???
            0ms - SourceBinder::to_module_def (1 calls)
            0ms - ???
        0ms - Semantics::analyze_impl (2 calls)
        0ms - add_missing_impl_members_inner (2 calls)
        0ms - classify_name_ref (1 calls)
        0ms - crate_def_map:wait (4 calls)
        0ms - descend_into_macros (2 calls)
       12ms - ???

[lnicola]

@matklad it might be worth adding some docs about the profiler output and when it merges calls.

[matklad]

The calls are merged when they are faster than the threshold.

And yeah, I have a suspicion that not everyone is as comortable with our profiling infra as I am. I wonder how new ppl discover profiling? Where should we put the docs?

[jonas-schievink]

(Btw, automatically wrapping the item in {} like you implemented for expression might help with that?)

Yeah, I had that idea too.

Also, good to know that this is caused by resolve errors, it seems like we need to limit the number of them that we report, similar to what we already do for parse errors.

[matklad]

Hm, if I am reading profile correctly, we are taking 3 seconds to display 2k errors. I'd expect displaying 2k of anything to be instant tbh

[stanciuadrian]

The calls are merged when they are faster than the threshold.

And yeah, I have a suspicion that not everyone is as comortable with our profiling infra as I am. I wonder how new ppl discover profiling? Where should we put the docs?

Consider adding that on Windows > should be escaped by ^ like this set RA_PROFILE=*^>20. Double quoting doesn't work since env::var gives a quoted string and parsing fails.

[matklad]

Yeah, that's exactly what I am talking about, it's more convenient to just specify profiling in settings :D

[matklad]

As a first step, a test along the lines of

https://github.com/rust-analyzer/rust-analyzer/blob/43ea1bb9b96138b7967385260a59ea2c02b1a2f6/crates/rust-analyzer/src/benchmarks.rs#L21-L23

should be added.

[jonas-schievink]

The parser recovers from the syntax error by making all following idents in the file part of the USE_TREE, which explains why there are >2000 errors about unresolved imports.

DefDiagnostic::add_to uses AstId::to_node, which is logarithmic, but then uses expand_use_item and a counter to walk the use item and locate the node to highlight. Since all diagnostics are in the same use item, this results in O(n²) runtime to convert every error.

[flodiebold]

IMO we shouldn't ever be sending that many errors for a single file. (Emacs' flycheck actually disables error checking when we return that many.)

[matklad]

Yeah, that’s definitely true. I wonder, does it mean that we just shouldn’t care about diagnostics being quadratic? I am torn on this one.

[jonas-schievink]

I think at the point where being quadratic matters, we've already emitted too many diagnostics to be helpful to the user.

In this case, we're also not quadratic in general, but only within each use item, which are never so large that it should be a problem. We just get unlucky with how the parser recovers.