Do not persist winforms data to disk by CyrusNajmabadi · Pull Request #46391 · dotnet/roslyn

CyrusNajmabadi · 2020-07-28T21:01:13Z

There is a possible race condition in this cde (originally documented) that could lead us to persist data to disk that does not match what the host thinks the state of things are. i.e. we compute something is a form, then remote that information over to VS. THe processing of that data on the VS side happens at some point in the future, but we still persist what we think VS knows at that point.

If this gets out of sync (i.e. because a message is lost) user might be in a broken state until they can do something that forces reprocessing.

jmarolf · 2020-07-29T21:50:32Z

...sualStudio/Core/Def/Implementation/DesignerAttribute/VisualStudioDesignerAttributeService.cs

        {
            _workQueue = new AsyncBatchingWorkQueue<DesignerAttributeData>(
-                TimeSpan.FromSeconds(1),
+                TimeSpan.FromMilliseconds(500),


what is the rationale for this change?

i actually reduce it even more in the next PR. the point of these delays is to allow us to hear about a large set of result from oop at once, and then report them at once to VS in one go. i.e. we don't want to hear about 100 files, and then send 100 calls to update VS (each which might hit the UI thread). Instead, it's better to hear about 100 files, then report it in one go to VS on the UI thread once.

So the goal is simply to give enough time to hear about a lot, but not so long that it feels slow to the user for thigns to update. 250ms seems to work well. It allows for hearing about tons of files, not saturating the UI thread, and providing low latency between hearing about changes and updating the UI with them. Note: my next PR adds a comment on this.

"250ms seems to work well."

The PR is changing it to 500ms though?

next pr reduces it further :)

jasonmalinowski

Looks OK and I'll never complain about the removal of the a bunch of storage stuff. A bit worried about the use of ConcurrentDictionary in a place or two where if various members here can be running in parallel strange things might happen.

jasonmalinowski · 2020-07-29T19:41:04Z

...s/Remote/ServiceHub/Services/DesignerAttribute/RemoteDesignerAttributeIncrementalAnalyzer.cs

        }

-        public override Task RemoveProjectAsync(ProjectId projectId, CancellationToken cancellationToken)
+        public override async Task RemoveProjectAsync(ProjectId projectId, CancellationToken cancellationToken)


Does this also get called if we unload the entire solution? I'm guessing we'll see a project removed notification for each project now but would be good to confirm under a debugger. I know the live unit testing code was running into some 'surprises' here and I wouldn't want us to fall into the same trap.

@mavasani for information here. IN the case of solution-unload, how to incremetnal analyzers hear about the 'removal'? Thanks!

Current implementation invokes RemoveDocumentAsync for each document in the solution: http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.cs,407. It seems it can certainly do batching here and just invoke RemoveProjectAsync for all projects in the solution.

on interesting. so there's no real point in responding to RemoveProject if i'm also responding to RemoveDocument, correct?

We invoke RemoveProject for enqueued project items for which solution.GetProject(id) returns null, i.e. project has been removed and we cannot find the documents for it.

http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.LowPriorityProcessor.cs,153

jasonmalinowski · 2020-07-29T22:00:47Z

...s/Remote/ServiceHub/Services/DesignerAttribute/RemoteDesignerAttributeIncrementalAnalyzer.cs

+            var changedData =
+                latestData.Where(d => !_documentToLastReportedInformation.TryGetValue(d.document.Id, out var existingInfo) || existingInfo.category != d.data.Category)
+                          .ToImmutableArray();


If the user had a file that we classified, and then edits the code such that ComputeLatestDataAsync no longer returns anything for that file, do we need to report the now lack of data? And clear out the dictionary in that case?

we do report the lack of data. specifically, we report changes from whatever we last reported. We don't actually clear out the dictionary because we want to store in teh dictionary the version of the last report, taht way we can ignore future events (that don't chanage hte state). If we remove things from teh dictionary, that is akin to saying "we've never reported anything", and then we will just keep notifying VS of "it's not designable, it's not designable, it's not designable".

Ah got it; all documents being queried still get added to the dictionary no matter what. Makes sense.

jasonmalinowski · 2020-07-29T22:02:00Z

...s/Remote/ServiceHub/Services/DesignerAttribute/RemoteDesignerAttributeIncrementalAnalyzer.cs


-                tasks.Add(ComputeDesignerAttributeDataAsync(
-                    storage, projectVersion, designerCategoryType, document, cancellationToken));
+                // If we don't have a path for this document, we cant proceed with it.


Suggested change

// If we don't have a path for this document, we cant proceed with it.

// If we don't have a path for this document, we can't proceed with it.

jasonmalinowski · 2020-07-29T22:03:18Z

...s/Remote/ServiceHub/Services/DesignerAttribute/RemoteDesignerAttributeIncrementalAnalyzer.cs

+                if (_documentToLastReportedInformation.TryGetValue(document.Id, out var existingInfo) &&
+                    existingInfo.projectVersion == projectVersion)


What's the threading guarantees here? Can RemoveDocumentAsync earlier in the file be running at the same time as this? Is there some risk that we'd remove the document, then process it here, and add it back?

it cannot be running at the same time. all the incremental-analyzer entrypoints are serialized. the only concurrency is from me spitting out tons of tasks (that i when-all) in one of those incremental-analyzer entrypoints.

@mavasani to confirm.

all the incremental-analyzer entrypoints are serialized

Yes, that is correct

jasonmalinowski · 2020-07-29T22:03:52Z

...s/Remote/ServiceHub/Services/DesignerAttribute/RemoteDesignerAttributeIncrementalAnalyzer.cs

                // We either haven't computed the designer info, or our data was out of date.  We need
                // So recompute here.  Figure out what the current category is, and if that's different
                // from what we previously stored.


Comment seems stale -- should be deleted now?

it's still accurate. we have to comptue this and compare against whatever we last reported. teh difference now is that we keep track of what we reported in memory instead of on disk.

Well, it's still correct, but previously that function was doing the storage comparison. Now this function is blissfully ignorant of how it's being used -- does the comment move elsewhere then?

i'll revise in next pr.

revised in next pr.

CyrusNajmabadi · 2020-07-29T22:12:37Z

Looks OK and I'll never complain about the removal of the a bunch of storage stuff. A bit worried about the use of ConcurrentDictionary in a place or two where if various members here can be running in parallel strange things might happen.

So a core benefit of IncrementalAnalyzer is that it makes its requests to us serially. I do think i could just make this a Dictionary. however, then i have to be very careful about my own reads/writes in the tasks i spin off to process all docs in a project in parallel.

jasonmalinowski · 2020-07-29T23:21:04Z

@CyrusNajmabadi Sounds good.

tmat · 2020-07-31T16:34:40Z

...s/Remote/ServiceHub/Services/DesignerAttribute/RemoteDesignerAttributeIncrementalAnalyzer.cs


-        private readonly IPersistentStorageService _storageService;
+        /// <summary>
+        /// Keep track of the last information we reported.  We will avoid notifying the host if we recompute and these


host [](start = 89, length = 4)

Should this rather say "the client", not "the host"?

tmat · 2020-07-31T16:38:53Z

...s/Remote/ServiceHub/Services/DesignerAttribute/RemoteDesignerAttributeIncrementalAnalyzer.cs

-            // easy it would be to get out of that state (just edit the file).
-
-            await PersistLatestInfosAsync(project.Solution, projectVersion, latestInfos, cancellationToken).ConfigureAwait(false);
+            // Now, keep track of what we've reported to the host so we won't report unchanged files in the future.


host [](start = 61, length = 4)

Client?

This reverts commit f4a419b, reversing changes made to 23e5b1c.

…ormPerf""

Revert "Revert "Merge pull request #46391 from CyrusNajmabadi/formPerf""

CyrusNajmabadi added 2 commits July 27, 2020 17:00

Prioritize designer scanning for the current file.

36ae7dd

Do not persist form information

c40fdc1

CyrusNajmabadi requested a review from a team as a code owner July 28, 2020 21:01

CyrusNajmabadi added 3 commits July 28, 2020 14:02

Remove copy

a4eae05

Adjsut

306c8e2

Simplify

4210f42

CyrusNajmabadi requested review from genlu and jasonmalinowski July 28, 2020 23:32

jinujoseph added the Area-IDE label Jul 29, 2020

CyrusNajmabadi mentioned this pull request Jul 29, 2020

Tweak delays we have before we start incrementally analyzing in OOP. #46426

Merged

jmarolf reviewed Jul 29, 2020

View reviewed changes

jasonmalinowski approved these changes Jul 29, 2020

View reviewed changes

CyrusNajmabadi merged commit f4a419b into dotnet:master Jul 29, 2020

ghost added this to the Next milestone Jul 29, 2020

CyrusNajmabadi deleted the formPerf branch July 29, 2020 23:37

CyrusNajmabadi mentioned this pull request Jul 30, 2020

Form1.cs doesn't get recognized as designer file for new WinForms .NET Core projects #45471

Closed

tmat reviewed Jul 31, 2020

View reviewed changes

JoeRobich added a commit that referenced this pull request Aug 4, 2020

Revert "Merge pull request #46391 from CyrusNajmabadi/formPerf"

f626ab2

This reverts commit f4a419b, reversing changes made to 23e5b1c.

CyrusNajmabadi mentioned this pull request Aug 4, 2020

Test revert of two recent WinForm perf PRs for RPS validation #46540

Merged

RikkiGibson added a commit that referenced this pull request Aug 4, 2020

Revert "Merge pull request #46391 from CyrusNajmabadi/formPerf"

f6862a8

This reverts commit f4a419b, reversing changes made to 23e5b1c.

CyrusNajmabadi added a commit to CyrusNajmabadi/roslyn that referenced this pull request Aug 6, 2020

Revert "Revert "Merge pull request dotnet#46391 from CyrusNajmabadi/f…

9ca7a3a

…ormPerf""

RikkiGibson added a commit that referenced this pull request Aug 7, 2020

Merge pull request #46548 from CyrusNajmabadi/revertRevert

9078075

Revert "Revert "Merge pull request #46391 from CyrusNajmabadi/formPerf""

RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020

	// If we don't have a path for this document, we cant proceed with it.
	// If we don't have a path for this document, we can't proceed with it.

		if (_documentToLastReportedInformation.TryGetValue(document.Id, out var existingInfo) &&
		existingInfo.projectVersion == projectVersion)

Conversation

CyrusNajmabadi commented Jul 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jul 29, 2020

Uh oh!

jasonmalinowski commented Jul 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants