made diagnostic tags to be removed faster and inserted slower. #8102
Conversation
experience seems actually quite better than before. anyway, 2 main changes are 1. active file analysis delay got shorten to 400ms from 800ms. 2. notification to editor on removed tags are now 50ms but added tags are now 1.5 seconds. ... more detail explanations below for #1. the delay change is only for 1 file (a file that has focus). all other file analysis delay is same as before (1.5 seconds). so I believe perf impact due to this should be fairly small. and it is still better than RTM which was 200ms. also, after RTM, we already made LB behavior not to be affected by this delay. so this change shouldn't affect LB behavior. for #2. tagger actually has many small delays in them. 2 main ones are 1) delay to produce tags 2) delay to notify editor about changed tags. 1) is to reduce doing repeated works to generate tags. for diagnostic tagger, this actually is not needed since unlike any other tagger, diagnostic tagger uses external service (diagnostic service) which already does all these things (basically #1 delay is logically doing what 1) is trying to do) now 1) delay is set to NearImmediate (50ms) 2) is to prevent us from abusing editors too much with a lot of notifications. basically this delay make sure we only ping editor once in a while (used to be 50ms) and aggregate events between them. now, 2) is split into 2 different delays. one for adding new tags and the other for removing old tags. adding new tags is now set to 1.5 seconds and removing old tags is set to 50ms.
|
@dotnet/roslyn-ide can you take a look? @CyrusNajmabadi @jasonmalinowski I didn't run tests yet. but want to get feedback while doing that. thank you. |
| private static void AddToList<T>(ref List<T> list, T item) | ||
| { | ||
| list = list ?? new List<T>(); | ||
| list.Add(item); |
There was a problem hiding this comment.
This optimization seems unnecessary.
There was a problem hiding this comment.
why bad to have it?
There was a problem hiding this comment.
Because it's a micro optimization that in general doesn't seem like it will do anything.
Also, a better optimization would be to pool. These lists may be very large. And they're going to be created, and then thrown away immediately when the DiffResult is created. If we pool them, then we only ever have as many lists created as needed to service the max number of threads doing diffing simultaneously.
|
Overall this looks quite promising. I need to look more closely tomorrow aobut that bit about diffing with oldTagTrees. I'm not totally convinced that the new code is correct. But it's too late now for me to tell at all :) |
| @@ -112,6 +112,9 @@ private sealed partial class TagSource : ForegroundThreadAffinitizedObject | |||
| RecalculateTagsOnChanged(new TaggerEventArgs(TaggerDelay.Short)); | |||
| } | |||
|
|
|||
| public TaggerDelay NewTagsNotificationDelay => _dataSource.AddedTagNotificationDelay; | |||
There was a problem hiding this comment.
These names shoudl match.
|
overall i think this is good. I just had small nits. If you change/validate them, then 👍 from me. @dotnet/roslyn-ide Anyone else want to take a look? |
|
I won't have a chance to look at the code for a bit, but my concern still comes down to whether people feel it slows down their ability to apply code fixes. I guess we'll need to dogfood it to see. |
|
@Pilchie Lightbulb is not controlled by this. if user gets cue from squiggle than user might wait for squiggle to show up, but if they get cue from LB, LB will show up exactly same time it used to be. since not all squiggle provide code fix, probably more people get cue from LB than squiggle? |
|
Ah, got it. It didn't register that this wasn't changing the diagnostic service itself. Objection withdrawn. |
|
LGTM |
made diagnostic tags to be removed faster and inserted slower.
experience seems actually quite better than before.
anyway, 2 main changes are
...
more detail explanations below
for #1. the delay change is only for 1 file (a file that has focus). all other file analysis delay is same as before (1.5 seconds). so I believe perf impact due to this should be fairly small. and it is still better than RTM which was 200ms. also, after RTM, we already made LB behavior not to be affected by this delay. so this change shouldn't affect LB behavior.
for #2. tagger actually has many small delays in them.
2 main ones are
delay to produce tags
delay to notify editor about changed tags.
is to reduce doing repeated works to generate tags. for diagnostic tagger, this actually is not needed since unlike any other tagger, diagnostic tagger uses external service (diagnostic service) which already does all these things (basically Initial port and addition of README.md #1 delay is logically doing what 1) is trying to do)
now 1) delay is set to NearImmediate (50ms)
now, 2) is split into 2 different delays. one for adding new tags and the other for removing old tags.
adding new tags is now set to 1.5 seconds and removing old tags is set to 50ms.