Tag based classification for C# by DavidKarlas · Pull Request #4740 · mono/monodevelop

DavidKarlas · 2018-05-07T15:59:03Z

This PR introduces 2 big changes:

Switches from MonoDevelopSourceTextContainer to TextBuffer.AsTextContainer() which enables some roslyn functionalities(Tagger among others)
Switches to VSEditor Tagger classification instead direct Roslyn one which brings huge performance improvement when typing

I will need to figure out how to optimize rendering logic, to not redraw lines if nothing changed, like VS2017 does...

…g created via `AsTextContainer` If MonoDevelop creates new instance of `SourceTextContainer`(`MonoDevelopSourceTextContainer`) and adds this one to workspace the one used by Roslyn services via `AsTextContainer` is disconnected and most services don't work... Change in `IMonoDevelopHostDocument.cs` is to fix exception(before was never called because `SourceTextContainer` was not connected)

…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

Now it's inserting lines in correct order so FormattedSpan can return First().Start,Last.End and also fixed issue with 1st and last line having different snapshot, this is not ideal solution.

DavidKarlas · 2018-05-07T16:02:17Z

Forgot to mention this fixes #3770

Therzok · 2018-05-07T20:59:14Z

main/src/addins/MonoDevelop.SourceEditor2/Mono.TextEditor/Gui/MdTextViewLineCollection.cs

+				if (this.Count == 0)
+					return new SnapshotSpan (textEditor.TextSnapshot, 0, 0);
+				var start = this [0].Start;
+				var end = this.Last ().EndIncludingLineBreak;


Please avoid linq. It usually does not work well perf-wise.

If you never use linq you don't do anything wrong.

mkrueger

Nice - we can really need that.

However this breaks span updates. /* & @". VS.NET taggers need to do that somehow.

ATM we've a more or less dirty work around for making it work with the roslyn classification highlighting.

And how are ISemanticChangeNotificationService notifications handled ? They seem to be missing here.

In roslyn there are some event sources that raise state change events:

https://github.com/dotnet/roslyn/blob/master/src/EditorFeatures/Core/Shared/Tagging/EventSources/TaggerEventSources.SemanticChangedEventSource.cs

(The old roslyn classification highlighting had that)

I'm not really sure how the span updates work according to
TaggerEventSources.TextChangedEventSource.cs they should always reclassify the whole buffer on text change which is something I try to avoid.

mkrueger · 2018-05-08T06:43:16Z

main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.TypeSystem/MonoDevelopWorkspace.cs

 				return document;
 			}
-			var monoDevelopSourceTextContainer = new MonoDevelopSourceTextContainer (this, documentId, editor);
+			var monoDevelopSourceTextContainer = editor.TextView.TextBuffer.AsTextContainer ();


Now we're starting to benefit from the vs.net editor work - nice :)

mkrueger · 2018-05-08T06:49:43Z

main/src/addins/MonoDevelop.SourceEditor2/VSEditor/TagBasedSyntaxHighlighting.cs

+				var styleName = GetStyleNameFromClassificationName (baseType.Classification);
+				if (styleName == null)
+					continue;
+				scope = new ScopeStack (styleName);


This doesn't look right the classification map already contains the scope and then you're using GetStyleNameFromClassificationName to produce a new scope ?

GetStyleNameFromClassificationName has to go and the missing entries need to be added to the classificationMap.
Maybe make a default classificationMap and make classificationMap an immutable dictionary.

If an entry is missing add the default case from GetStyleNameFromClassificationName to the ClassificationMap. So the ScopeStack objects get re-used instead of re-created every time.

I fixed this now by returning of mapping one is found, also, all of this is cached so it shouldn't be a problem.

mkrueger · 2018-05-08T06:50:14Z

main/src/addins/MonoDevelop.SourceEditor2/VSEditor/TagBasedSyntaxHighlighting.cs

+					continue;
+				scope = new ScopeStack (styleName);
+			}
+			return classificationTypeToScopeCache [classificationType] = scope ?? new ScopeStack (EditorThemeColors.Foreground);


Do not create a new ScopeStack (EditorThemeColors.Foreground) more than once. But it's not a big issue because of the cache.

mkrueger · 2018-05-08T06:54:16Z

main/src/addins/MonoDevelop.SourceEditor2/VSEditor/TagBasedSyntaxHighlighting.cs

+			if (classificationTypeToScopeCache.TryGetValue (classificationType, out var cachedScope))
+				return cachedScope;
+			ScopeStack scope = null;
+			foreach (var baseType in classificationType.BaseTypes.Concat (classificationType)) {


Wouldn't it be more performant to start with the classificationType and then check the base types ?
And in which order are the base types ? And what's with the BaseTypes of the BaseTypes ?

But in general it doesn't make much sense to me - in which cases no classification for a classification type is done - we should always have a color for a classification and if not we should log it. Then we can go to the base types.

About performance, we are caching this so it's not really problem, plus we have to check all anyway.
Base types are in alphabetic order, priorty is handled differently in VSEditor, see https://github.com/dotnet/roslyn/blob/88d1bd1/src/EditorFeatures/Core.Wpf/Classification/ClassificationTypeFormatDefinitions.cs
I'm now handling BaseTypes of BaseTypes...

Logging non-found classifcaiton is imo not an option because some classifications are created on the fly when merging multiple classifications...

mkrueger · 2018-05-08T07:05:59Z

main/src/addins/MonoDevelop.SourceEditor2/VSEditor/TagBasedSyntaxHighlighting.cs

+
+		private ScopeStack GetScopeNameFromClassificationType (IClassificationType classificationType)
+		{
+			if (classificationTypeToScopeCache.TryGetValue (classificationType, out var cachedScope))


classificationTypeToScopeCache is an immutable dictionary. Let's say you've a vb.net and c# file open.
Is the IClassificationType keyword the same for vb.net and c# ? If so it would return a wrong value for one of these files.

This object and cache is created per ITextView so I don't expect this to be an issue.

…changes e.g. `/*`

DavidKarlas · 2018-05-08T09:34:52Z

I updated PR with However this breaks span updates. /* & @". VS.NET taggers need to do that somehow. fixed.

slluis · 2018-05-08T11:38:59Z

Tell me more, what is that huge performance improvement?

DavidKarlas · 2018-05-08T12:48:35Z

Often times when I see laggy typing I profile it and it turns out to be problem like seen at #3770.
Problem is that we call await document.GetSemanticModelAsync () on UI thread every time user types new character(since we want to render), most times this is incrementally parsed and most things cached hence it's fast to return, but sometimes it can take few hundred milliseconds or even seconds...

monodevelop/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.Editor.Highlighting/RoslynClassificationHighlighting.cs

Line 199 in b315922

    
           var classifications = Classifier.GetClassifiedSpans (await document.GetSemanticModelAsync ().ConfigureAwait (false), span, workspace, cancellationToken);

The way new logic work is same as in VS Windows, it uses SnapshotSpan.TranslateTo to translate older classification to latest textbuffer so it's "instantly" renders best approximation and later calls classifier.ClassificationChanged when newer classification is available instead of blocking UI thread to figure out all classifications(like we did before)...

… removed

mkrueger · 2018-05-08T15:11:54Z

Still missing the ISemanticChangeNotificationService usage.

However testing semantic updates seem to work for some reason. I think that the code analysis may do the updates since it takes a while for highlighting to correct. But it works so I approve.

One thing: remove the tests for the span updater I already did that. And we should consider this for d15-7 because it's a better fix to my span updater move. The tag based highlighter needs to work anyways.

DavidKarlas · 2018-05-08T15:18:17Z

ISemanticChangeNotificationService is used here: http://source.roslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/Classification/SemanticClassificationViewTaggerProvider.cs and that is notifying us via classifier.ClassificationChanged.

Tnx for heads ups about Unit tests, I noticed locally few minutes ago that it doesn't compile... I removed them now.

David Karlaš added 3 commits May 7, 2018 17:06

Improve FormattedSpan implementation on TextViewLineCollection

3f64ccd

Now it's inserting lines in correct order so FormattedSpan can return First().Start,Last.End and also fixed issue with 1st and last line having different snapshot, this is not ideal solution.

DavidKarlas requested a review from mkrueger as a code owner May 7, 2018 15:59

DavidKarlas requested review from KirillOsenkov and Therzok May 7, 2018 15:59

Therzok reviewed May 7, 2018

View reviewed changes

mkrueger mentioned this pull request May 8, 2018

Fixes issue #4716 TextChanged line highlighting logic needs to be #4737

Closed

mkrueger suggested changes May 8, 2018

View reviewed changes

mkrueger reviewed May 8, 2018

View reviewed changes

Addressed PR comments and fixed problem with not updating big syntax …

a0fff87

…changes e.g. `/*`

Therzok approved these changes May 8, 2018

View reviewed changes

DavidKarlas changed the title ~~Tag based classification~~ Tag based classification for C# May 8, 2018

Remove obsolete unit tests, since they were testing class that is now…

b8b039b

… removed

mkrueger self-requested a review May 8, 2018 15:09

mkrueger approved these changes May 8, 2018

View reviewed changes

mkrueger mentioned this pull request May 8, 2018

Fixes issue #4716 TextChanged line highlighting logic needs to be #4738

Closed

Therzok merged commit 8fd9d24 into master May 8, 2018

DavidKarlas deleted the tagBasedClassification branch May 8, 2018 17:11

DavidKarlas mentioned this pull request May 15, 2018

GetSemanticModelAsync should not be called from UI Thread #3770

Closed

garuma mentioned this pull request May 16, 2018

Semantic classification has regressed in master #4820

Closed

KirillOsenkov mentioned this pull request May 21, 2018

Fix https://github.com/mono/monodevelop/issues/4820 dotnet/roslyn#27007

Merged

Conversation

DavidKarlas commented May 7, 2018

Uh oh!

DavidKarlas commented May 7, 2018

Uh oh!

Therzok May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkrueger May 8, 2018

Choose a reason for hiding this comment

Uh oh!

mkrueger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkrueger May 8, 2018

Choose a reason for hiding this comment

Uh oh!

mkrueger May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidKarlas May 8, 2018

Choose a reason for hiding this comment

Uh oh!

mkrueger May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidKarlas May 8, 2018

Choose a reason for hiding this comment

Uh oh!

mkrueger May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidKarlas May 8, 2018

Choose a reason for hiding this comment

Uh oh!

mkrueger May 8, 2018

Choose a reason for hiding this comment

Uh oh!

DavidKarlas May 8, 2018

Choose a reason for hiding this comment

Uh oh!

DavidKarlas commented May 8, 2018

Uh oh!

slluis commented May 8, 2018

Uh oh!

DavidKarlas commented May 8, 2018

Uh oh!

mkrueger commented May 8, 2018

Uh oh!

DavidKarlas commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Therzok May 7, 2018 •

edited

Loading

mkrueger left a comment •

edited

Loading

mkrueger May 8, 2018 •

edited

Loading

mkrueger May 8, 2018 •

edited

Loading

mkrueger May 8, 2018 •

edited

Loading

DavidKarlas commented May 8, 2018 •

edited

Loading