Roslyn: SyntacticClassificationTaggerProvider should not block UI thread

[vasily-kirichenko]

I experience huge UI freezes as I edit large F# files (~15K lines):

As I understand, the root cause of this is blocking wait for semantic classification spans here https://github.com/dotnet/roslyn/blob/84bc7eea2e05e9d9d7e1b53c2a107c0ac9857a72/src/EditorFeatures/Core/Implementation/Classification/SyntacticClassificationTaggerProvider.TagComputer.cs#L308

Yes, F# can calculate fresh classification for long time (up to 30 seconds and more on huge files), so I believe Roslyn should not block UI thread waiting for data, making the asynchronous API meaningless.

VS 2017 RC2

[CyrusNajmabadi]

This is (currently) by design. The expectation is that if you supply a "IEditorClassificationService" that you will be able to classify the document with similar performance characteristics as, say, C#/VB/TS/JS would be able to. If you cannot implement that interface efficiently, then that will have negative downstream consequences.

Can you help me understand why the impl of IEditorClassificationService is so slow? You mentioned "Yes, F# can calculate fresh classification for long time (up to 30 seconds and more on huge files)". We shoudl be asking for some smallish sub-span of the document. So it should be quick to be able to get the syntactic classifications for that span.

Thanks!

--

Note: if you cannot supply classifications quickly here, we can change it so that SyntacticClassificationTaggerProvider is not exported for F#. But you'll have to supply your own classifier in that case.

[CyrusNajmabadi]

As I understand, the root cause of this is blocking wait for semantic classification spans here

Note: this is the Syntactic classifier, not the semantic one. The SemanticClassificationViewTaggerProvider should never block. THe core assumption here is that for any Roslyn language, being able to syntactically process a small portion of a file should not be hugely expensive. If it is, then our design assumptions don't fit htat language, and that language will need to supply it's own impls for operations like this.

[vasily-kirichenko]

Thanks for explanation.

[CyrusNajmabadi]

Do you know why the impl is so expensive on the F# side? is it really that costly to get a syntax tree and classify a portion of it?

[vladima]

syntactic classifications should not be too expensive to compute. Semantic classification thought can take some time since especially for large files at the bottom of the project.
@vasily-kirichenko can you please share this snapshot since it is interesting what compiler was doing at this timeframe?

[vasily-kirichenko]

@vladima That was on my branch where I broke the lever cache and the whole file was relexed on each key press. I've fixed it and everything is ok now. My main concern here is that the API advitises asyncronicy, but in reality it's called synchronously on the UI thread. Either make it synchronous (no Task as result type) or call it from a background thread.

[CyrusNajmabadi]

Sometimes we do block on asynchronous calls. It's part of our design. We aren't good at making it clear what things are pure sync and can take any amount of time, versus what may occasionally be synv, and should always try to be fast

[vasily-kirichenko]

OK, it's not reproduced on modern machines, but it is on older ones and on virtual machines. @gmpl shared a dotTrace snapshot here https://github.com/Microsoft/visualfsharp/issues/2104#issuecomment-269245446 (link https://drive.google.com/file/d/0B-QdEbD9b0kPQXZ0akktTFIyVVE/view?usp=sharing), which shows that the UI freeze is caused by https://github.com/dotnet/roslyn/blob/84bc7eea2e05e9d9d7e1b53c2a107c0ac9857a72/src/EditorFeatures/Core/Implementation/Classification/SyntacticClassificationTaggerProvider.TagComputer.cs#L308 and it's not the F# classification provider who is slow. Look the whole freeze is waiting for classified spans:

Why we don't give them so long? It seems there are a lot of threads that wants to proceed, but CPU is too slow / where are too few cores and only one thread is running for all that time:

I'm not even sure the top methods are related to classification, it's just some type checking in F# compiler service initiated by whatever reason.

My suggestion: do not make the blocking call to IEditorClassificationService.AddSyntacticClassificationsAsync. A language service may not be written very well, so it may produce syntactic classification not as fast as Roslyn team expects, but the end user should not experience UI freezes, ever.

[Pilchie]

@CyrusNajmabadi - can you take another look at the data above and see what can be done for F# in the short term?

[CyrusNajmabadi]

Probably the best thing to do here is to not use the SyntacticClassificationTaggerProvider for f# syntactic classification. We should only export it for C#/VB/TS/JS. F# can then do all it's classification in "AddSemanticClassificationsAsync". Our expectation is that syntactic work is fast, and semantic work is slow. If all F# work is slow, then it should all go through the AddSemanticClassificationsAsync path.

[Pilchie]

Can F# split out it's lexical classification that is fast and do that in the Syntactic classifier and do the rest in the Semantic classifier? @vasily-kirichenko you implied on the other issue that if Lexical was requested that would be okay?

[CyrusNajmabadi]

agree with @Pilchie . The split in the API is essentially between what we think is the 'fast path' and hte 'slow path'. If F# can't really provide a fast-path, then just providing the slow path would be acceptable.

[vasily-kirichenko]

F# classification _is_ split into fast and slow parts. The fast one is based not even on untyped tree, but on lexer, which should be light fast. I will investigate why it's slow here.

[Pilchie]

Moving to 2.1, since we're past the point where we can make changes like this for RTW, but I'd like to continue the conversation to see what we can do on either side.

[vasily-kirichenko]

@Pilchie so RTM will use 2.1?

[Pilchie]

No, RTW will use 2.0 - but we are out of time to take any changes here for RTW.

[forki]

What is RTW meaning?

Am 23.01.2017 18:36 schrieb "Kevin Pilch" [e-mail]:

No, RTW will use 2.0 - but we are out of time to take any changes here for
RTW.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/roslyn/issues/16109#issuecomment-274559494,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNIvlfDya820MbjHeaNNL8KQ3VVi9ks5rVOUVgaJpZM4LWH-f
.

[Pilchie]

"Release to Web". The new name for RTM.

[forki]

Yeah why would one stick to something that's widely accepted. But thanks!

Am 23.01.2017 18:37 schrieb "Kevin Pilch" [e-mail]:

"Release to Web". The new name for RTM.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/roslyn/issues/16109#issuecomment-274559939,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNNxIFwjqJR-oFwoeWkeTc-k9Y-Ogks5rVOVmgaJpZM4LWH-f
.

[vasily-kirichenko]

It's solver in VS 2017 RTW.