Don't suggest names for errors in normal IDE usage

[cartermp]

Fixes #6044

This turns off name suggestion for errors in the IDE except for Build, instead using Completion as a source of names for suggesting errors in a Roslyn code fix.

The following will still generate suggestions on build errors:

• fsc.exe
• fsi.exe
• Build within VS
• FSI within VS
• dotnet build

However, names will not be continuously generated when editing code in the IDE and triggering this logic due to having typed an unresolved identifier. This should resolve the current (as of latest dev16.0 branch) biggest source of CPU and memory usage in normal IDE usage for larger solutions.

[forki]

I don' really think disabling it is the way to go.
Did you check #6049 ?

[cartermp]

I captured my thoughts on that PR but I'll restate the important bit here:

Fundamentally, this feature is simply not built with IDE tooling in mind. It's great for a batch compile job/command line builds, but for a long-running process like a language server it's simply calculating and re-calculating too many things. The feature needs two "modes":

• The current one for batch compile jobs
• An out-of-process spellchecker routine that uses a tree (such as Roslyn's which uses a BKTree) to traverse a populated set of symbols independently of the in-proc work

The latter is not something that FCS can do yet. We're going to pursue this sort of architecture in the future, but not right now.

Disabling this by default while we are still in the VS process for this sort of work is the right thing to do.

[forki]

With the same logic you basically need to disable almost every useful feature. There is always some cost and recalculation involved. I mean I understand that this is something that we need to balance - that's why I really wanted to know the impact of that other PR. Am Mo., 7. Jan. 2019, 19:12 hat Phillip Carter <notifications@github.com> geschrieben:

…

I captured my thoughts on that PR but I'll restate the important bit here: Fundamentally, this feature is simply *not* built with IDE tooling in mind. It's great for a batch compile job/command line builds, but for a long-running process like a language server it's simply calculating and re-calculating too many things. The feature needs two "modes": - The current one for batch compile jobs - An out-of-process spellchecker routine that uses a tree (such as Roslyn's which uses a BKTree <http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Utilities/SpellChecker.cs>) to traverse a populated set of symbols independently of the in-proc work The latter is not something that FCS can do yet. We're going to pursue this sort of architecture in the future, but not right now. Disabling this by default while we are still in the VS process for this sort of work is the right thing to do. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6063 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNMYQi0Lc45sfKvbOWXraeydTuN-xks5vA44YgaJpZM4Zuupn> .

[cartermp]

That's a false equivalence.

[vasily-kirichenko]

Excellent, but I think fsc.exe should not generate suggestions by default either. It should be a pure-IDE, optional feature.

[majocha]

Yes, this looks like the proper way to deal with it.
The code that computes suggestions should be called by the FSharpReplaceWithSuggestionCodeFixProvider instead.

This way the impact should be so low that I don't see the need to disable this feature at all.

[cartermp]

It'll be heinous to decouple the logic that generates candidates:

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L1831-L1838

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L1845-L1851

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2209-L2248

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2354-L2392

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2478-L2514

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2577-L2606

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2681-L2696

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2800-L2805

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2821-L2826

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2844-L2852

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2873-L2878

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2885-L2890

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L2920-L2933

https://github.com/Microsoft/visualfsharp/blob/a7acf9f4198550f697945ba36c8321e96541eeab/src/fsharp/NameResolution.fs#L3055-L3104

I'm not particularly keen on doing that but I suppose it's possible. Lots and lots of plumbing values to/from NameResolution if we want the code fix to be able to get the same fidelity of results.

A different way could be picked, but something basic like checking matches against all symbols is going to be expensive (I've tested it and it is SLOW), and may not give the same results. If that were crippled to just be within the current document it wouldn't be bad, but then the feature would be inconsistent with using the IDE build.

I'm also not in favor of ripping out this feature for fsc/normal build. It's a valuable feature that I wouldn't be pressing on if it weren't affecting CPU and memory so much in the IDE.

[dsyme]

I've noticed this feature to be useful in the IDE. But I've also noticed the computation to be oddly expensive.

[forki]

Even if I disagree with disabling this feature by default: I want to thank you folks for working so hard on perf. I think the community really appreciates that kudos! Also big thanks to @davkean for profiling work.

[cartermp]

@forki Thanks 🙂 - I've actually now changed the code fix to be on by default, but I'd like to play with it a bit more to see what the impact is. This means the code is run differently, so it could mean that the algorithm isn't triggered as much.

[cartermp]

As of the current state of this PR (code fix on by default), the allocations in #6044 are practically gone:

And CPU usage doesn't even register. So I think this PR solves the problem. However, suggestions are generated differently than with Build. It is equivalent to invoking completion. Is that considered fine?

[forki]

Cool. So ctrl+shift b would still trigger it? How about saving a file?

[cartermp]

@forki So unfortunately due to #6036 you won't see ctrl+B unless you toggle "Build Only":

We really need to fix that bug for reasons beyond this, but when it is fixed you should probably see both in the error list.

Saving a file does not show it, nor is it in the QuickInfo tooltip (we don't control that, unfortunately). But the fixer icon is in the tooltip area:

And of course ctrl+. will trigger it as well.

[cartermp]

@TIHan @KevinRansom this is ready

[TIHan]

Looks great. Not a fan of booleans as arguments, but we can clean that stuff up later. This is worth having to get rid of allocations.