Reduce lsp server allocations by ~70% during load of already restored projects - #84205
Reduce lsp server allocations by ~70% during load of already restored projects#84205dibarbet wants to merge 2 commits into
Conversation
…urrent ProjectDependencyHelper.NeedsRestore parsed the entire project.assets.json into a Newtonsoft DOM on every project open to detect unresolved package references, accounting for ~800 MB-1 GB of the ~1.96 GB allocated during a Roslyn.slnx load. Add a fast path that reads the sibling project.nuget.cache via the public CacheFileFormat.Read API. When the cache is valid (format version matches, last restore succeeded, dependency-graph hash recorded) and every package reference the project currently declares is present with a satisfying version in the cache's ExpectedPackageFilePaths, the restore is known to be current and the assets parse is skipped. Any reference not covered (e.g. a package added to the csproj or Directory.Packages.props since the last restore) falls back to the precise LockFileFormat.Read assets check, so the fast path can only accelerate the already-restored case and never masks a stale restore. Also bump NuGet.ProjectModel 6.8.0-rc -> 6.14.0 so the assets-file fallback uses the System.Text.Json-based reader. End-to-end Roslyn.slnx load allocations drop from 1,956.6 MB to 649.4 MB (-66.8%); a 313-project micro-benchmark confirms 0 spurious restores. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes restore-up-to-date detection during Language Server project load by avoiding expensive project.assets.json parsing when possible, and updates the NuGet APIs used by the language server.
Changes:
- Adds a fast-path that reads NuGet’s
project.nuget.cacheand verifies declaredPackageReferenceitems are satisfied by restored packages, falling back toproject.assets.jsonparsing when the cache is missing/invalid/inconclusive. - Refactors package-reference resolution logic into a shared helper to avoid duplication between the cache and assets-file paths.
- Updates
NuGet.ProjectModelto6.14.0(switching to newer NuGet APIs that reduce allocations during assets parsing).
Show a summary per file
| File | Description |
|---|---|
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs | Introduces project.nuget.cache fast-path restore validation and shares package resolution logic across cache/assets checks. |
| eng/Packages.props | Updates NuGet.ProjectModel package version to 6.14.0 for the language server. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
| return false; | ||
| } | ||
|
|
||
| return CheckAssetsFileForUnresolvedReferences(projectFileInfo, projectAssetsPath, logger); |
There was a problem hiding this comment.
just to check my understanding, what would happen if we just return true; in this path?
There was a problem hiding this comment.
This would restore. The risk is that checking the cache file could mis-report a package as missing (we're parsing a version and id from the path). If we just return true here it could lead to a restore loop.
There was a problem hiding this comment.
So is the idea that certain project setups, will consistently fail to produce a project.nuget.cache that we can use to verify that restore was complete+successful?
When does that failure happen? Is it limited to old tooling versions, unusual project setups, ...?
There was a problem hiding this comment.
Essentially I am not 100% confident we'll always be able to exactly match packages from the cache file, given that we have to parse the package id and versions from the file path.
I haven't been able to come up with an exact scenario where it breaks, but I can easily imagine how it could break (nuget path changes to not include version, some kind of mismatch in package path and package id, casing differences, encoding differences, etc).
So I left the original code in there as a defensive fallback (which is still better than before due using STJ).
One alternative is to instead hand-roll a parser for the project.assets.json and only use that - I think we could get similar allocations as using the cache file, but we'd no longer be using the nuget API.
jasonmalinowski
left a comment
There was a problem hiding this comment.
Seems fine; I'm interested though in what it'd look like if we knew we could trust the NuGet cache, or if they just had an API we could call to do all this checking for us.
| /// This intentionally does not reproduce NuGet's dependency-graph-hash comparison, which needs the current | ||
| /// restore dependency graph that is not available here. Instead it confirms the restored package set covers the | ||
| /// project's declared references and otherwise defers to the precise assets-file check, so it can only ever | ||
| /// accelerate the up-to-date case and never report a stale restore as current. |
#84205 had discussion on the best path forward. Until that is resolved we should just upgrade the version so we use the STJ reader for the project.assets.json ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/dotnet/roslyn/pull/84618)
|
alternative approach - #84647 |
…ences (#84647) Alternative approach to #84205. `ProjectDependencyHelper` called `LockFileFormat.Read` to materialize the whole `project.assets.json` into a `LockFile` model purely to answer one question: is every `PackageReference` resolved? That parses and allocates the entire file when only the keys of the top-level `libraries` object are needed. Where #84205 avoids the cost by reading `project.nuget.cache` instead, this keeps reading `project.assets.json` (so there is no dependency on a second file staying in sync) and instead makes reading it cheap. ## Approach `ProjectAssetsReader` streams the file through a pooled 16 KiB buffer with `Utf8JsonReader`, tracking depth so only the top-level `libraries` keys are examined. Keys are `Name/Version`, which is all that is needed to decide whether a `PackageReference` is satisfied. Two details keep allocation flat rather than proportional to file size: - Library keys are decoded onto the stack with `CopyString` instead of `GetString()`. On a large assets file (~1,500 libraries) `GetString()` alone would allocate 130-190 KB. - `FileStream` is constructed with `bufferSize: 1`. It otherwise lazily allocates its own 4 KiB internal buffer once reads are smaller than that, which cancels out the pooling. The buffer grows if a single JSON token does not fit. No real assets file needs this (the longest token across the 302 assets files in this repo is a few hundred bytes), but a valid file must not be rejected just because a token is unusually large. ## Benchmark Added `ProjectAssetsReaderBenchmarks` to `IdeCoreBenchmarks`, over a checked-in 29 KB assets file taken from a real project in this repo. It is `#if NET` since `LockFileFormat` is only used on the .NET Core language server path. Run with `--inProcess`; the default BenchmarkDotNet toolchain regenerates a project that rebuilds the Roslyn graph and races itself on `artifacts/obj`. | Method | Median | Gen0 | Allocated | | --- | ---: | ---: | ---: | | `CheckUpToDateWithLockFileModel` (before) | 1.48 ms | 7.81 | 99,875 B | | `CheckUpToDateWithStreamingReader` (after) | 1.16 ms | - | 922 B | **Allocation drops by ~99% and no Gen0 collections remain.** Allocation was confirmed with a separate deterministic harness (explicit GC, 20 warmup + 200 measured iterations, `GC.GetAllocatedBytesForCurrentThread`), which independently measured 100,349 B vs 633 B per check. Timing is the less reliable half of this table: it moved around with machine load across runs, with the streaming reader landing between 0.6x and 0.9x of baseline. Allocation is the durable result here, not the speedup. Measured end to end, loading this repo's solution in the language server went from 340,849,616 to 328,875,544 bytes allocated (**-11.4 MiB, -3.5%**), which is in line with ~99 KB saved per project across the projects that have package references. ## Behavior Unchanged, and covered by 21 tests: missing assets file, projects with no package references (file is not read at all), case-insensitive package ids, exact versions and version ranges, unparseable ranges, malformed files (still throw, as `LockFileFormat.Read` did), a `libraries` value that is valid JSON but not an object, project libraries, library names spanning a buffer boundary, tokens larger than the buffer, and a UTF-8 BOM. Two of those are worth calling out because they were verified against the old code path rather than assumed: - `Utf8JsonReader` does not consume a byte order mark, but `LockFileFormat` did, so the reader strips one explicitly to avoid rejecting a file that used to load. - A `libraries` value that is valid JSON but not an object (`[]`, `null`, a string) is read as "no libraries" rather than throwing. `LockFileFormat` behaved the same way, so this reports the references as unresolved exactly as before. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44b92b40-08d1-4664-b2b5-ada0e946bf10
Reduces total allocations in the language server process for loading an already restored Roslyn.slnx by ~70% (650 MB):
The allocations in the baseline were caused by the nuget API using Newtonsoft to parse the
project.assets.json, for every project. We did this to determine if the project had been restored (allPackageReferenceitems in the project present in theproject.assets.json).This change has two parts
Update nuget API package
The latest nuget API package uses STJ and reduces the allocations to about ~200 MB (from ~650 MB) by itself.
Use
project.nuget.cacheWe reduce allocations even further by reading the
project.nuget.cacheto find theExpectedPackageFilePathsand verify it contains thePackageReferenceitems. This avoids reading the much largerproject.assets.jsonfile. However it is not as robust as it requires parsing the package Id and version from the file path. So on misses, we fall back to the previousproject.assets.jsoncheck.Microsoft Reviewers: Open in CodeFlow