Implement DocumentHighlightsService

[vasily-kirichenko]

It ignores documentsToSearch parameter as I'm not sure what it's for, maybe look at it in the future.

[vasily-kirichenko]

OK, it was a wrong ImportingConstructorAttribute. It now works:

[cartermp]

Awesome

[vasily-kirichenko]

It's ready

[forki]

💋

[dsyme]

This whole change looks great

1. I'm concerned that we need to get a measure on how adding new FCS requests like this is changing the overall quality of the tools. Every new request that goes into the FCS queue means a change in the status quo, though it feels really hard to get a grip on the impact of the change

• There is a risk of placing too much "stress" on the FCS queue, particularly if these requests are never cancelled. While this request is running, the FCS queue is unavailable to serve other requests.

• If those other requests were effectively the same request anyway, then that's not such a big deal. Indeed the request can actually help fill the caches for the current document, so that when you're moving he cursor around, the cache gets populated for the current document. This might mean the next press of "." or goto-definition or whatever responds more quickly.

One specific thing is to understand whether Roslyn issues a cancellation for the request if the cursor moves into a different area, or if the user changes visible files. If it does, that helps make sure the FCS queue is not being choked up with useless work.

Perhaps @vladima has experience w.r.t. performance testing for the typescript implementation, which also uses a single workqueue.

[vasily-kirichenko]

@dsyme

• I'm pretty sure Roslyn cancels outdated requests, it would be weird if it did not having a cancellation token passed everywhere.
• We have such a tagger implemented in VFPT without cancellation and it works totally OK, see https://github.com/vasily-kirichenko/FSharpVSPowerTools/blob/master/src/FSharp.Editing.VisualStudio/Symbol/HighlightUsageTagger.fs

[dsyme]

We have such a tagger implemented in VFPT without cancellation and it works totally OK

Yep, I use it all the time. I think it's actually a help because of the warm-the-cache-when-the-user-interacts effect

But what I'd really like is a methodology where we actually knew the effects of these changes through automation. Really we want to replay 100 typical editing sessions on small and big projects and take some statistics (e.g. longest UI pause, average reaction time etc.). But that's essentially impossible to do in an automated way.

[vasily-kirichenko]

AFAIK there should not be any UI pauses at all (unless there is a GC pressure), because all Roslyn API is asynchronous. It does not matter what we do in background threads, the UI should stay responsive and it's guaranteed by core Roslyn code we plugged into. The fact that it's used by C#/VB editors as well makes some confidence that it interacts with UI thread carefully and it's battle tested since VS 2015. So I think this PR brings a better tagger than we have in VFPT.

[dsyme]

AFAIK there should not be any UI pauses at all (unless there is a GC pressure), because all Roslyn API is asynchronous. It does not matter what we do in background threads, the UI should stay responsive and it's guaranteed by core Roslyn code we plugged into.

My experience in using VS2017 RC (master or bleeding-edge) is that there are a lot of UI blocks (at least when using a large solution which might indicate GC pressure). One known one seems to have been Ctrl-Space, though that's being fixed. But I'm seeing lots of non-responsiveness.

But I agree that this PR should generally be OK.

[cartermp]

@dsyme Could you try and enumerate some of these cases in an issue? I think it would be really helpful to have some of these tracked. I've noticed some related to msbuild blocking the UI thread when f5'ing, but generally my issues have more been related to things like typechecking not being complete.

[vasily-kirichenko]

@KevinRansom please merge it, it includes tryClassifyAtPosition (good) fix.

[KevinRansom]

@vasily-kirichenko
can you take care of the merge issue please, then I will pull

Thanks

Kevin

[vasily-kirichenko]

Done.

[vasily-kirichenko]

It was broken, I've committed fix.