Stream project.assets.json when checking for unresolved package references - #84647
Conversation
…ences ProjectDependencyHelper used LockFileFormat.Read to materialize the entire project.assets.json into a LockFile model just to answer whether every PackageReference had been restored. That parses and allocates the whole file (~400 KB for a typical project) when only the keys of the top level "libraries" object are needed. Replace it with ProjectAssetsReader, which streams the file through a pooled 16 KiB buffer with Utf8JsonReader and inspects only the library keys. Library keys are decoded onto the stack via CopyString so allocation does not scale with the number of libraries in the file. Behavior is unchanged: missing files, projects without package references, case insensitive package ids, version ranges, and malformed files all behave as before. Also adds a benchmark for this scenario to IdeCoreBenchmarks over a representative real world assets file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44b92b40-08d1-4664-b2b5-ada0e946bf10
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR replaces the language server’s project.assets.json “are all PackageReferences resolved?” check from fully materializing NuGet’s LockFile model to a streaming Utf8JsonReader pass that only inspects the top-level libraries keys, reducing allocations on project load. It also adds a BenchmarkDotNet comparison and unit tests to validate behavior and edge cases (buffer boundaries, oversized tokens, BOM).
Changes:
- Add
ProjectAssetsReaderto streamproject.assets.jsonand mark resolved package references without materializing a full lock-file model. - Update
ProjectDependencyHelperto use the streaming reader and pooledbool[]tracking rather than NuGet’sLockFileFormat.Read. - Add benchmarks + checked-in benchmark data and add unit tests for malformed inputs and boundary conditions.
Show a summary per file
| File | Description |
|---|---|
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs | Switch unresolved-dependency detection to the new streaming reader with pooled tracking. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectAssetsReader.cs | New streaming reader over project.assets.json that scans only the top-level libraries keys. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/ProjectDependencyHelperTests.cs | New unit tests covering missing file, malformed JSON, version/range matching, buffer-boundary cases, and BOM handling. |
| src/Tools/IdeCoreBenchmarks/IdeCoreBenchmarks.csproj | Link the language server reader into the benchmarks project for .NET TFMs and add needed references/usings. |
| src/Tools/IdeCoreBenchmarks/Lsp/ProjectAssetsReaderBenchmarks.cs | New benchmark comparing lock-file model parsing vs streaming reader. |
| src/Tools/IdeCoreBenchmarks/Lsp/ProjectAssetsReaderBenchmarkData.json | Checked-in representative assets data used by the benchmark. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 3
|
So far it sounds like this approach would be strictly better than #84205? Is that accurate? |
Pros: Cons: |
- Use Encoding.UTF8.Preamble instead of a collection expression. The reader is linked into IdeCoreBenchmarks, which pins LangVersion 11, so a C# 12 feature broke that project's build. - Correct the comment about library key shapes. Project libraries use the same "Name/Version" keys as packages, and the lock file model matched against both, so the shape check is only a filter for keys belonging to neither. - Cover a "libraries" value that is valid JSON but not an object, and a project library, both of which match the previous LockFileFormat behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44b92b40-08d1-4664-b2b5-ada0e946bf10
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/ProjectDependencyHelperTests.cs:42
- An empty/whitespace
project.assets.jsonis malformed JSON, but the new streaming reader will currently treat it as having no libraries (and just report all references unresolved) instead of throwingJsonException. Since this PR keeps the existing expectation that malformed assets files throw, it would be good to add a dedicated regression test for the empty-file case (and adjust the reader if needed) to lock the behavior down.
[Fact]
public void NeedsRestore_MalformedAssetsFileThrows()
{
var projectAssetsPath = WriteAssetsFile("""{"libraries":{"Package/1.0.0":{}}""");
Assert.ThrowsAny<JsonException>(() => NeedsRestore(projectAssetsPath, ("Package", "1.0.0")));
}
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
| return true; | ||
| } | ||
|
|
||
| if (projectFileInfo.PackageReferences.Length == 0) |
There was a problem hiding this comment.
Should we check this first, since the absence of a project.assets file will cause an unnecessary restore?
There was a problem hiding this comment.
even if there are no package references, if there is no project.assets.json we still need to restore (ensure core lib references are resolved)
LockFileFormat.Read catches all parse failures, logs, and returns a lock file with no libraries, so a corrupt or partially written assets file reported a restore. The streaming reader let JsonException escape into project load instead. Catch it in the caller, log an error, and report a restore so a rewrite of the file recovers. The reader now also reports the top level "version" property so it can be included in that error. It is passed by reference because an out parameter would not be definitely assigned when parsing throws, which is the case the value is needed for. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 44b92b40-08d1-4664-b2b5-ada0e946bf10
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectDependencyHelper.cs:98
- The error log on assets read failure drops the exception object, which can make diagnosing failures harder (no stack/exception type in logs). Consider logging the exception via the ILogger overload, and include the exception type in the formatted details placeholder.
logger.LogError(e, string.Format(
LanguageServerResources.Failed_to_read_project_assets_file_0_version_1_2,
projectAssetsPath,
assetsFileVersion?.ToString() ?? "<unknown>",
e.Message));
- Files reviewed: 20/20 changed files
- Comments generated: 1
| } | ||
|
|
||
| state = reader.CurrentState; | ||
|
|
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectAssetsReader.cs:101
- This example JSON comment line contains a tab character before the closing
], which is inconsistent with the repo’s no-tabs convention and can cause whitespace/formatting noise. Replace the tab with spaces.
// ]
- Files reviewed: 20/20 changed files
- Comments generated: 0 new
RikkiGibson
left a comment
There was a problem hiding this comment.
I'm still interested in reviewing, but, no need to wait on my sign off. Just interested to know more about how the change works
sounds good, I'll merge this and can address any feedback you have in a followup. |
Alternative approach to #84205.
ProjectDependencyHelpercalledLockFileFormat.Readto materialize the wholeproject.assets.jsoninto aLockFilemodel purely to answer one question: is everyPackageReferenceresolved? That parses and allocates the entire file when only the keys of the top-levellibrariesobject are needed.Where #84205 avoids the cost by reading
project.nuget.cacheinstead, this keeps readingproject.assets.json(so there is no dependency on a second file staying in sync) and instead makes reading it cheap.Approach
ProjectAssetsReaderstreams the file through a pooled 16 KiB buffer withUtf8JsonReader, tracking depth so only the top-levellibrarieskeys are examined. Keys areName/Version, which is all that is needed to decide whether aPackageReferenceis satisfied.Two details keep allocation flat rather than proportional to file size:
CopyStringinstead ofGetString(). On a large assets file (~1,500 libraries)GetString()alone would allocate 130-190 KB.FileStreamis constructed withbufferSize: 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
ProjectAssetsReaderBenchmarkstoIdeCoreBenchmarks, over a checked-in 29 KB assets file taken from a real project in this repo. It is#if NETsinceLockFileFormatis 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 onartifacts/obj.CheckUpToDateWithLockFileModel(before)CheckUpToDateWithStreamingReader(after)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.Readdid), alibrariesvalue 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:
Utf8JsonReaderdoes not consume a byte order mark, butLockFileFormatdid, so the reader strips one explicitly to avoid rejecting a file that used to load.librariesvalue that is valid JSON but not an object ([],null, a string) is read as "no libraries" rather than throwing.LockFileFormatbehaved the same way, so this reports the references as unresolved exactly as before.