Do not persist diagnostics just to purge them from memory by CyrusNajmabadi · Pull Request #53792 · dotnet/roslyn

CyrusNajmabadi · 2021-05-31T18:00:00Z

WIP to see if there are any issues here.

I recommend reviewing a commit at a time.

CyrusNajmabadi · 2021-05-31T18:20:09Z

@mavasani I wanted to open this to continue the discussion started in: #53743

Effectively, it seems strange to me that diagnostics are using hte persistence service in teh first place (esp. with how they use it). Specifically, it does not appear as if we could ever really successfully load diagnostics from a previous session. That's because we are using versionstamps here, and versionstamps are not stable across sessions of VS. All other usages from all other features (except diagnostics) use checksums for this reason as those are stable and are keyed to the exact contents of the entity we are associating the persisted data with.

Furthermore, even if this was supported, it would be weird as we'd effectively be hoping that the persisted diagnostics were valid against the solution we loaded, which could lead to incorrect information being presented.

Because of this, and because of hte 64bit work, and because diagnostics should be OOP, i've opened this PR to remove all usages of persisting for diagnostics. Instead, the last results for any particular analyzer+key combo are just held in memory until the next time we compute the updated values for it.

This seems like this should not be problematic given the size and scope of diagnostics. In other words, it's common to literally have zero diagnostics much of the time. ANd even in bad situations where a lot of errors are produced (likely through missing references/cascading) you should only have thousands of them. And they will eventually go away once the latest are generated. So it feels like persisting this stuff (esp. as it's in OOP) and esp in a 64 bit world just seems like an unnecessarily expensive and complex thing to do for something conceptually simple.

Thoughts on your part about this?

CyrusNajmabadi · 2021-05-31T18:21:32Z

src/EditorFeatures/Test/Diagnostics/DiagnosticAnalyzerServiceTests.cs

            var location = Location.Create(document.FilePath, textSpan: default, lineSpan: default);

-            await service.SynchronizeWithBuildAsync(
+            service.SynchronizeWithBuild(


a lot of things became sync as we only needed async for persistence/loading.

CyrusNajmabadi · 2021-05-31T18:28:16Z

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs


                // loading data can be cancelled any time.
-                var serializer = new DiagnosticDataSerializer(_owner.AnalyzerVersion, lastResult.Version);
+                var serializerVersion = lastResult.Version;


in general, where we used to use a Serializer (which points at a 'Version' and assocaited teh diagnostics with that version), we now just pas along the version instead.

CyrusNajmabadi · 2021-05-31T18:29:41Z

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs

                if (lastResult.IsDefault)
                {
-                    return await LoadInitialAnalysisDataAsync(persistentService, project, cancellationToken).ConfigureAwait(false);
+                    return await LoadInitialAnalysisDataAsync(project, cancellationToken).ConfigureAwait(false);


it's unclear to me if this even makes sense. what are we loading initial data from? that said, i'm not sure the lifetimes of proejct-states, so it's possible this has meaning otuside of the concept of persistence across sessions.

CyrusNajmabadi · 2021-05-31T18:30:35Z

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs

-                => SaveCoreAsync(project, result, persistentService);
-
-            public Task SaveInMemoryCacheAsync(Project project, DiagnosticAnalysisResult result)
-                => SaveCoreAsync(project, result, persistentService: null);


these collapsed down to a single concept.

CyrusNajmabadi · 2021-05-31T18:30:55Z

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs

                    }

-                    if (!await TryDeserializeDocumentDiagnosticsAsync(persistentService, serializer, document, builder, cancellationToken).ConfigureAwait(false))
+                    if (!TryGetDiagnosticsFromInMemoryStorage(serializerVersion, document, builder))


names were updated to better reflect what it's doing.

CyrusNajmabadi · 2021-05-31T18:31:39Z

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.StateManager.cs

                return stateSets.ToImmutable();
            }

-            public static bool OnDocumentReset(IEnumerable<StateSet> stateSets, TextDocument document)


these static methods moved to be local functions at the single locations where they were called.

CyrusNajmabadi · 2021-05-31T18:32:16Z

...res/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_BuildSynchronization.cs

-                                nameof(SynchronizeWithBuildAsync),
-                                () => state.SaveAsync(PersistentStorageService, project, result),
-                                cancellationToken);
+                        state.SaveToInMemoryStorage(project, result);


we were both saving to in-memory cache and saving to persistence layer. now that the latter is gone, this is jsut a single call with no task queueing.

CyrusNajmabadi · 2021-05-31T18:34:12Z

src/VisualStudio/Core/Def/Implementation/TaskList/ExternalErrorDiagnosticUpdateSource.cs

            // Update the state to clear diagnostics and raise corresponding diagnostic updated events
            // on a serialized task queue.
-            _taskQueue.ScheduleTask(nameof(ClearErrors), async () =>
+            _taskQueue.ScheduleTask(nameof(ClearErrors), () =>


aside: i think we should get rid of this taskqueue. I have a feeling it is the source of lots of potential bugs around having a consistent model of all diagnostics. esp. as these are all just sync operations, i really dont' get why we dont' just apply them synchronously.

I believe the task queue was introduced to minimize the work done in these callbacks, which is likely to slow down the build. Won't all these code go away from Roslyn once we move to LSP diagnostics?

taht would be the hope :) we certainly shouldn't have an ExternalErrorDiagUpdateSource in roslyn then :)

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs

mavasani · 2021-06-01T10:37:56Z

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs

+                    : default;
            }

            private void RemoveInMemoryCache(DiagnosticAnalysisResult lastResult)


Is this method still used?

yes. effectively as we produce new results, this is used to purge the previous values we had associated with that analyzer.

This is the code in question:

public void SaveToInMemoryStorage(Project project, DiagnosticAnalysisResult result) { Contract.ThrowIfTrue(result.IsAggregatedForm); Contract.ThrowIfNull(result.DocumentIds); RemoveInMemoryCache(_lastResult); // save last aggregated form of analysis result _lastResult = result.ToAggregatedForm();

mavasani · 2021-06-01T10:47:02Z

Overall, the change looks good to me. However, @heejaechang introduced the persistent storage for diagnostics for performance reasons, so I would let him provide additional context here on the kind of performance issues that were seen to motivate this storage model for diagnostics.

CyrusNajmabadi · 2021-06-01T14:32:27Z

Given that we are now oop and 64bit, I feel like any motivation here would be way down.

especially when there are large number of diagnostics for things such as broken references.

This is potentially interesting. But even then, I'm not sure we should default to adding lots of complexity to support that scenario, given that it should be rare and temporary.

Any thoughts @heejaechang

CyrusNajmabadi · 2021-06-02T00:32:58Z

@mavasani for review. note that i'll be doing an RPS pass first to ensure everything is ok.

sharwell · 2021-06-02T03:54:17Z

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs

            }

-            private async Task SerializeAsync(IPersistentStorageService? persistentService, DiagnosticDataSerializer serializer, Project project, TextDocument? document, object key, string stateKey, ImmutableArray<DiagnosticData> diagnostics)
+            private void AddToInMemoryStorage(VersionStamp serializerVersion, Project project, TextDocument? document, object key, string stateKey, ImmutableArray<DiagnosticData> diagnostics)


💭 I wouldn't have hated to see this stay asynchronous. In the event we did need to offload this data, we would have the option of implementing it on the original asynchronous paths using temporary storage service instead of persistent storage service. We can still do that, but the change would involve reverting a decent portion of this PR.

@sharwell . That is reasonable. I'll make this all async again (probably ValueTask). We can then easily tweak in the future if appropriate.

CyrusNajmabadi · 2021-06-02T17:47:40Z

RPS test passed: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/328173

CyrusNajmabadi · 2021-06-02T18:48:39Z

Took sam's suggestion. so if we need to move to another persistence system, we can do so simply.

CyrusNajmabadi added 3 commits May 31, 2021 10:53

Do not persist diagnostics just to purge them from memory

879ef60

Rename members for clarity

c34fa7a

REmove unused code

f944fc1

CyrusNajmabadi requested a review from mavasani May 31, 2021 18:00

ghost added the Area-IDE label May 31, 2021

MOve static methods

de666d2

CyrusNajmabadi commented May 31, 2021

View reviewed changes

Remove unused field

4aeccc3

CyrusNajmabadi commented May 31, 2021

View reviewed changes

mavasani reviewed Jun 1, 2021

View reviewed changes

src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs Show resolved Hide resolved

mavasani reviewed Jun 1, 2021

View reviewed changes

CyrusNajmabadi marked this pull request as ready for review June 1, 2021 15:09

CyrusNajmabadi requested a review from a team as a code owner June 1, 2021 15:09

CyrusNajmabadi requested a review from mavasani June 2, 2021 00:32

Inline further

87bf7c2

mavasani approved these changes Jun 2, 2021

View reviewed changes

sharwell approved these changes Jun 2, 2021

View reviewed changes

CyrusNajmabadi added 2 commits June 2, 2021 11:22

Make async again

7e06c9a

Make async again

253a00d

CyrusNajmabadi added 2 commits June 2, 2021 11:41

Cancellation tokens

5cabbd1

Simplify

97d2d16

CyrusNajmabadi enabled auto-merge June 2, 2021 18:48

CyrusNajmabadi merged commit 75d1282 into dotnet:main Jun 2, 2021

ghost added this to the Next milestone Jun 2, 2021

CyrusNajmabadi deleted the diagnosticsPersistence branch June 2, 2021 20:27

CyrusNajmabadi mentioned this pull request Jun 21, 2021

Getting Diagnostics is slower on 64-bit #53743

Closed

RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021

Conversation

CyrusNajmabadi commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented May 31, 2021

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani commented Jun 1, 2021

Uh oh!

CyrusNajmabadi commented Jun 1, 2021

Uh oh!

CyrusNajmabadi commented Jun 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jun 2, 2021

Uh oh!

CyrusNajmabadi commented Jun 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi commented May 31, 2021 •

edited

Loading