Restore: improve DependencyGraphSpec.Save(...) performance #3215
Conversation
| /// Every call to WriteObjectStart must be balanced by a corresponding call to WriteObjectEnd. | ||
| /// </summary> | ||
| /// <exception cref="ObjectDisposedException">Thrown if this object is disposed.</exception> | ||
| void WriteObjectStart(); |
There was a problem hiding this comment.
Last time, someone complained about this breaking change: NuGet/Home#8080.
Since then they have fixed their dependency https://github.com/dotnet/arcade/pull/2664/files, but we need to be aware that people do take dependencies.
We can say, the client tooling comes first though :)
There was a problem hiding this comment.
We just observed this breaking change in dotnet/runtime#48462 after upgrading the SDK to 6.0. You could argue that our custom packaging code should be replaced by NuGet's pack task, but we aren't there yet.
There was a problem hiding this comment.
What is interesting about this is, even though the change was merged in Feb 2020, it didn't make it into 5.0.100. Was that intentional? The fix for us in dotnet/arcade is just to reference newer nuget packages to update what we bind against.
There was a problem hiding this comment.
This change is in 5.0.100, https://github.com/NuGet/NuGet.Client/blob/release-5.8.x/src/NuGet.Core/NuGet.Packaging/RuntimeModel/IObjectWriter.cs
Or at least should be according to the branch and tags.
Can you help me understand the problem you are seeing and why you think it's not in 5.0.100.
There was a problem hiding this comment.
I think @nkolev92 was commenting that this is breaking because they added a member to an interface. We aren't broken by that addition. I think the break we're currently seeing is something different.
There was a problem hiding this comment.
In addition to what Nikolche wrote, there are a few other breaking API changes.
In IObjectWriter, WriteObjectInArrayStart was removed, as well as in the classes that implement the interface, so anyone currently using it will break.
JsonObjectWriter's default constructor was removed, and a new constructor with a parameter was added, and the GetJson, GetJObject, WriteTo and GetPreviousContainer methods were removed.
The only possible solution I can think of is to create new classes for the improved functionality,. But that doesn't feel like a good solution, so maybe living with the breaking change is sufficient. It's a shame we have this leaky abstraction.
Had to update the NuGet package dependencies because of a breaking change in their API: NuGet/NuGet.Client#3215 (comment).
Had to update the NuGet package dependencies because of a breaking change in their API: NuGet/NuGet.Client#3215 (comment).
Had to update the NuGet package dependencies because of a breaking change in their API: NuGet/NuGet.Client#3215 (comment).
Bug
Fixes: NuGet/Home#9031
Regression: No
Fix
Details:
DependencyGraphSpec.Save(…)saves a dependency graph spec (DG spec) to disk. The previous implementation serialized theJObjectrepresentation of the DG spec into a string and then wrote the string to disk. This intermediate step of serializing to a string is both unnecessary and a performance issue. As project and solution DG specs grow beyond 80 KB, these strings will be allocated on the large object heap (LOH). This has terrible performance implications given that NuGet restore is triggered automatically in many scenarios (i.e.: build, rebuild, debug, code analysis, etc.).Testing/Validation
Tests Added: Yes
Validation: Using the NuGet.Client solution as a case study, I profiled performance 5 times before and 5 times after this change. More iterations than 5 would have been great, but it's time consuming.
Deleting only *.nuget.dgspec.json files before each test (to force calls to
DependencyGraphSpec.Save(…)), I observed the following "no-op-ish" restore performance changes in Visual Studio. I say "no-op-ish" because there's no real scenario where only these files are deleted; however, the scenario is no-op in all other respects: a full restore was performed previous to testing. It's also important to note that these numbers are based on _all) activity in VS during a restore operation not just NuGet activity; there are many variables which I cannot control within VS.All numbers below are averages.
A real-life no-op restore --- that is one without me deleting *.nuget.dgspec.json files outside of Visual Studio --- allocated 776,496 bytes on the LOH. With this change it's 0.