Skip to content

Cache DocumentDiagnosticAnalyzer results by version stamp to avoid redundant typecheck on unchanged documents - #20121

Open
xperiandri wants to merge 1 commit into
dotnet:mainfrom
xperiandri:fix-document-diagnostic-analyzer-cache
Open

Cache DocumentDiagnosticAnalyzer results by version stamp to avoid redundant typecheck on unchanged documents#20121
xperiandri wants to merge 1 commit into
dotnet:mainfrom
xperiandri:fix-document-diagnostic-analyzer-cache

Conversation

@xperiandri

Copy link
Copy Markdown
Contributor

Fixes #20120

Summary

FSharpDocumentDiagnosticAnalyzer.GetDiagnostics previously recomputed syntax/semantic diagnostics (parse, typecheck, UnusedParentheses) on every crawler pass, even when the document text and project state had not changed since the last computation.

This adds a version-stamp-aware cache keyed by struct (DocumentId * DiagnosticsType), storing the last computed (textVersion, projectVersion, ImmutableArray<Diagnostic>) tuple:

  • For Syntax diagnostics, only the document textVersion is tracked (projectVersion uses VersionStamp.Default).
  • For Semantic diagnostics, both textVersion and document.Project.GetDependentVersionAsync() are tracked.
  • If the current versions match the cached entry, the cached diagnostics are returned directly, skipping parse/typecheck/UnnecessaryParenthesesDiagnosticAnalyzer work entirely.

This mirrors the versioned-cache pattern used for referenced-project compilation emission in FSharpProjectOptionsManager.fs (emitCache).

Testing

  • dotnet build vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj -c Debug — succeeded.
  • Deployed to RoslynDev hive (Build.cmd -c Debug -deployExtensions) and validated with a CPU trace of devenv.exe.

…recomputing diagnostics when document/project version is unchanged
@xperiandri xperiandri changed the title Cache DocumentDiagnosticAnalyzer results by version stamp to avoid redundant typecheck on unchanged documents Cache DocumentDiagnosticAnalyzer results by version stamp to avoid redundant typecheck on unchanged documents Aug 3, 2026
@xperiandri
xperiandri marked this pull request as ready for review August 3, 2026 00:33
@xperiandri
xperiandri requested a review from a team as a code owner August 3, 2026 00:33
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Aug 3, 2026

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.

document.Project.Solution.GetFSharpExtensionConfig().ShouldProduceDiagnostics()

static let cache =
ConcurrentDictionary<struct (DocumentId * DiagnosticsType), VersionStamp * VersionStamp * ImmutableArray<Diagnostic>>()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unbounded static cache / memory retention. This static ConcurrentDictionary is keyed by struct (DocumentId * DiagnosticsType) and has no eviction path. Edits to the same document overwrite their own key, so growth is bounded by the number of distinct documents ever opened (×2 for Syntax/Semantic) — but entries for documents that are closed or removed from the solution are never freed and persist for the entire lifetime of the devenv.exe process. Each retained entry holds an ImmutableArray<Diagnostic> (with Location/file-path data), so in a long-running session over a large solution this is steady, unbounded retention.

Consider evicting on workspace changes — e.g. subscribe to Workspace.WorkspaceChanged and drop entries on DocumentRemoved/DocumentRemovedFromSolution (and optionally when a document is closed), or cap the cache size. The referenced emitCache in FSharpProjectOptionsManager.fs is keyed by project reference and thus has far smaller cardinality than per-document diagnostics, so the retention surface here is larger.

@T-Gro
T-Gro self-requested a review August 3, 2026 19:21
@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

P1: DocumentDiagnosticAnalyzer repeats typecheck/diagnostics work on every crawler pass (no version-stamp cache)

2 participants