Fix memory leaks #11655
Fix memory leaks #11655
Conversation
This change brings back the behavior to hold the generated RazorCodeDocument produced by a document snapshot in a WeakReference. This avoids memory retention problems when document snapshots are rooted in the Roslyn workspace by the DynamicFileInfo system.
Now that GeneratedOutputSource holds its RazorCodeDocument in a WeakReference, there's not much point in keeping it on DocumentState. Instead, it can move to DocumentSnapshot, which is really more appropriate since GeneratedOutputSource needed a DocumentSnapshot to perform computation.
The BackgroundDocumentGenerator doesn't need to force compilation before updating the DynamicFileInfo system with a document snapshot. When the Roslyn workspace receives the DynamicFileInfo, compilation will occur through the TextLoader.
ProjectSnapshot.TryGetDocument(...) turns out to be a significant source of dictionary growth. However, we always know the number of documents in a particular ProjectSnapshot from its ProjectState, so we can set the capacity up front.
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentState.cs
Show resolved
Hide resolved
...r/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/Sources/GeneratedOutputSource.cs
Outdated
Show resolved
Hide resolved
|
cc @phil-allen-msft for awareness. Phil, I suspect you'll want to pull this change into the next 17.14 preview. Update (3/25/2025)The Good NewsThe test insertion shows several improvements, which is exciting! It also fixes the Razor completion test, which regressed in January after PR #11320 was merged.
The Bad NewsUnfortunately, this change also introduces a significant regression that I need to look at and solve before this can merge.
For what it's worth, this regression really just brings that test back to what it was reporting before PR #11320. Here's what the last six months look like:
So, it could be argued that we should take the regression to get the improvements. However, I've got a few ideas to try first to see if I can fix the regression and (hopefully) keep the improvements. In addition, note that the regression is in "9990.Totals", which tracks the entire test and not the scenarios. This test includes two scenarios: "Load solution (arctic)", and "0100. Full Search in Solution Explorer", but neither of those scenarios indicates a regression. 🤔 |
DustinCampbell
force-pushed
the
fix-memory-leaks
branch
from
1727bec to
7181112
Compare
Update (3/27/2025)I attempted to fix the SolutionExplorerSearchPerfTests.TestSolutionExplorerFullSearch regression but it only improved it a bit and erased other wins. Given that, I've rolled this PR back to what are essentially the changes that were already reviewed by @davidwengier and @ryzngard. |
|
I've grabbed a couple of memory snapshot screenshots to demonstrate the memory savings. I opened two common Razor solutions and waited for them to load: OrchardCore and MudBlazor. There are two screenshots for each solution: one without the fix where Razor objects dominate the heap, and one with the fix that shows Razor objects are barely present. OrchardCore - No Fix
OrchardCore - Fixed
MudBlazor - No Fix
MudBlazor - Fixed
|
|
I chatted with @phil-allen-msft, and we're going to go ahead and merge this. It potentially adds more churn to the effort to get the next VS insertion for Razor merged. However, only one regression is expected related to this change, and the improvements are very strong. |
While working on #11655, I noticed that `TagHelperBinder` accounts for a surprising amount of memory. This pull request aims to clean it up, reduce the amount of memory it holds onto, and improve the performance of binding operations. I recommend reviewing commit-by-commit as most commits have descriptions to aid reviewers. CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2675893&view=results Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/623866
4 participants
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2403339 (https://developercommunity.visualstudio.com/t/Razor-Still-Struggles-with-Memory-Utiliz/10862202)
This pull request brings back the behavior of weakly holding onto Razor generator output. While holding onto the
RazorCodeDocumentworks well in many cases, there are cases where document snapshots are held onto for a long time, resulting in an effective memory leak. The worst leak is caused by theDynamicFIleInfosystem. Razor provides aTextLoaderand various document services to Roslyn viaDynamicFileInfothat hold onto aDocumentSnapshot. And, because theDocumentSnapshot.Projecthas a reference to theProjectSnapshot, the entire project tree of documents is kept alive.WeakReference<RazorCodeDocument>inGeneratedOutputSource.GeneratedOutputSourcefromDocumentStatetoDocumentSnapshot.BackgroundDocumentGeneratorbefore updatingDynamicFileInfo. Compilation will happen when requested via theTextLoaderby Roslyn.ProjectSnapshotsets the capacity when creating its map ofDocumentSnapshots, since we know exactly how many documents it will contain.Initial Changes (3/24/2025)
CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2671111&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/621987
Update (3/25/2025)
CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2671715&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/622255
Update (3/27/2025)
CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2673040&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/622895
Final(?) Update (3/27/2025)
CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2673386&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/623124