Skip to content
This repository has been archived by the owner on Oct 4, 2021. It is now read-only.

GetSemanticModelAsync should not be called from UI Thread #3770

Closed
DavidKarlas opened this issue Feb 4, 2018 · 6 comments
Closed

GetSemanticModelAsync should not be called from UI Thread #3770

DavidKarlas opened this issue Feb 4, 2018 · 6 comments
Labels
Area: IDE Platform\Legacy Editor Performance vs-sync
Milestone
7.6

Comments

@DavidKarlas
Copy link
Member

DavidKarlas commented Feb 4, 2018 •
edited by xamarin-release-manager
Loading

https://github.com/mono/monodevelop/blob/ddb4355/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.Editor.Highlighting/RoslynClassificationHighlighting.cs#L137

Found this issue via UIThread hangs monitor...

VS bug #569967
@mkrueger
Copy link
Contributor

It's not called from ui thread - it's called async with await. I'm not sure if the hangs monitor is the issue here.

@DavidKarlas
Copy link
Member Author

It is called from UI thread from gtk OnExposeEvent method:
image

Even if it's called with await, there is no guarantee(or even promise) it won't execute on same thread.

@mkrueger
Copy link
Contributor

It's executed on the same thread when the semantic model is up2date and cached.
Creating always a task would be too much overhead. I'll discuss that with the roslyn guys.

@mkrueger
Copy link
Contributor

We call it always on UI thread like that - if that it would be a problem we would block all the time. So I doubt it's an issue.

@mkrueger
Copy link
Contributor

ATM I don't see a way to fix that - and no issue here. If you think you can improve that - try it out.
IMO it would just cause redraw flickers and not improve the UI in any way. That call can't block the UI thread and the semantic info is needed for the classifications since the roslyn classifiers are not 2 step like our old system was :(.

DavidKarlas added a commit that referenced this issue May 7, 2018
@DavidKarlas
Switched from RoslynClassificationHighligthing to TagBasedSyntaxHighl…
209e5a9 
…ighting fixes Issue #3770

This improves typing performance a lot in some cases when `GetSemanticModelAsync` takes long time(sometimes it can take multiple seconds)...
This uses advanced `AbstractAsynchronousTaggerProvider` from Roslyn that takes care of snapshot `TranslateTo` so if new classification is not ready yet, it translates old spans to requested snapshot and notifies editor when semantic classification is updated for specific span so it re-renders line containing updated classification.
There are two hacks in this commit:
 - Translations from `ClassificationTypes` to `ScopeStack` are hardcoded and so is order(same as in existing Roslyn classifier)
 - On `classifier.ClassificationChanged` event is ignored if span is whole file, reason for this is that it seems to me like SyntaxHighligher is doing this(probably can't figure out exact spans), Kirill debugged this on VS2017 and saw same behaviour, I suspect VS2017 doesn't re-render if classifications are same as in previous pass, hence it doesn't hurt them as much as it does MonoDevelop
@DavidKarlas DavidKarlas mentioned this issue May 7, 2018
Merged
@DavidKarlas DavidKarlas reopened this May 7, 2018
@xamarin-release-manager xamarin-release-manager added this to the 15.8 milestone May 8, 2018
Therzok pushed a commit that referenced this issue May 8, 2018
@DavidKarlas @Therzok
Switched from RoslynClassificationHighligthing to TagBasedSyntaxHighl…
5f064cb 
…ighting fixes Issue #3770

This improves typing performance a lot in some cases when `GetSemanticModelAsync` takes long time(sometimes it can take multiple seconds)...
This uses advanced `AbstractAsynchronousTaggerProvider` from Roslyn that takes care of snapshot `TranslateTo` so if new classification is not ready yet, it translates old spans to requested snapshot and notifies editor when semantic classification is updated for specific span so it re-renders line containing updated classification.
There are two hacks in this commit:
 - Translations from `ClassificationTypes` to `ScopeStack` are hardcoded and so is order(same as in existing Roslyn classifier)
 - On `classifier.ClassificationChanged` event is ignored if span is whole file, reason for this is that it seems to me like SyntaxHighligher is doing this(probably can't figure out exact spans), Kirill debugged this on VS2017 and saw same behaviour, I suspect VS2017 doesn't re-render if classifications are same as in previous pass, hence it doesn't hurt them as much as it does MonoDevelop
@DavidKarlas
Copy link
Member Author

This was fixed with #4740

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Area: IDE Platform\Legacy Editor Performance vs-sync
Projects
None yet
Milestone
7.6
Development

No branches or pull requests
4 participants
@mkrueger @sgmunn @DavidKarlas @xamarin-release-manager