Remove PackageDependencyGroupConverter #5872

martinrrm · 2024-06-26T03:55:58Z

Bug

Regression? Last working version:

Description

The goal of this PR is to avoid using JsonUtilities.LoadJson, for that we are removing the converter and let Newtonsoft.Json handle the deserialization of PackageDependencyGroup

This converter is used in the PM UI when we are deserializing the registration API, specifically when we select a package and we retrieve all the packages versions. https://learn.microsoft.com/en-us/nuget/api/registration-base-url-resource#package-dependency

PR Checklist

martinrrm · 2024-06-26T03:59:09Z

This is my first time doing a perf issue, what should do to confirm this is an improvement?

Also, I tried doing something similar like Parse the assets file with System.Text.Json by jgonz120 · Pull Request #5627 · NuGet/NuGet.Client (github.com), but unfortunately if we want to stop using Newtonsoft.json it will require more work in PackageSearchMetadata
.

src/NuGet.Core/NuGet.Protocol/Converters/PackageDependencyGroupConverter.cs

approved by mistake.

kartheekp-ms · 2024-06-27T03:49:48Z

This is my first time doing a perf issue, what should do to confirm this is an improvement?

@martinrrm A PerfView trace before and after the fix would help to understand the performance improvement resulting from the changes proposed in this PR.

martinrrm · 2024-06-27T20:20:59Z

Before

After

Steps:

Load Nuget.Client solution
Open PM UI for Solution
Select first installed package

I'm not 100% sure if I'm reading this right, but looks like there is an improvement 😄

zivkan · 2024-07-01T07:47:23Z

@martinrrm did you take that from PerfView's CPU Stacks view, or GC Heap Alloc view?

In any case, I think a BenchmarkDotNet app would be a simpler comparison, as you can microbenchmark just the converter, and the memory diagnoser will tell you exactly how much memory is allocated.

src/NuGet.Core/NuGet.Protocol/Converters/PackageDependencyGroupConverter.cs

martinrrm · 2024-08-12T18:50:55Z

Hi! I'm reopening the issue, I was finally able to work on it and run some benchmarks. Sorry for delay 😞

Benchmarks

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
ConverterWithoutLoadJson	19.16 us	0.134 us	0.125 us	0.8545	-	21.18 KB
ConverterUsingLoadJson	28.62 us	0.129 us	0.121 us	1.2512	0.0610	31.11 KB

Looking at the numbers by removing the usage of LoadJson and avoid using NJ.LINQ we can improve allocations by 31.91%

Branch with the benchmarks: dev-martinrrm-benchmarks

src/NuGet.Core/NuGet.Protocol/Converters/PackageDependencyGroupConverter.cs

zivkan

Can you please

Update the PR title and description to something more accurate for what this PR has become (especially since it's isolated to one specific type. it doesn't fix all usage of LoadJson across the entirety of NuGet.Client repo)
- Be careful when merging the PR too. The default commit message is set by the PR's title at the time the PR was opened, not when the merge button is clicked.
Provide fresh benchmark results for deserializing some registration blob

thanks, this PR makes me happy! less custom code.

zivkan · 2024-09-10T02:25:17Z

src/NuGet.Core/NuGet.Packaging/Core/PackageDependencyGroup.cs

+        [JsonConstructor]
+        private PackageDependencyGroup(NuGetFramework targetFramework)


I don't understand how the JSON deserialization sets the packages list, if this is the json constructor. But there are tests that pass? 😕

It's weird, since there is already a constructor that takes targetFramework and dependencies and null checks them I didn't want to change the behavior of it.

Previous converter would initialize an empty list for dependencies when it was null (no dependencies property in the json) thats why I added a constructor that takes only the TargetFramework that helps me create the empty list, this constructor is called only when deserialization happens. If we add a [JsonConstructor] it first attempts to use it and then populate the rest of the properties that have [JsonProperty]. JsonProperty doesn't needs a constructor to work.

The properties are get only, and they're not even auto-props. If Newtonsoft.Json is using reflection to set the value directly on the backing store, I don't understand how it can determine what the backing field name is. 🤷

src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs

zivkan · 2024-09-10T02:41:16Z

src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs

+    {
+        public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
+        {
+            serializer.Serialize(writer, value.ToString());


nitpick: this is... complicated 😆

The TFM value most people are used to reading, is not the value returned by .ToString(), but the value returned by .GetShortFoldername(). And for some reason, .nuspec files use a 3rd format, which is what we'll probably see from package sources when deserializing dependency groups.

But since this is a generic NuGetFramework converter, it's hard to say what's "correct" to use. However, I don't know of any scenario where NuGet will serialize this, so this method will probably never be used.

I wonder if we can use this overload of JsonConverterAttribute to pass data to the converter about which format type is preferred for the specific file.

I like your comment but since this is targeted for 17.2 P3 I believe I don't have enough time for this at this time, do you want me to investigate this further for next release?

Honestly, I think my preference would be to throw on write, like the old converter did. If anything needs to convert a NuGetFramework to a string in the future, then we can either investigate that "parameterized" converter, or create one converter per string format.

But I can also live with using ToString.

Should we throw a not implemented exception here if there are going to be no callers for this method? We can address Andy's feedback in a follow up PR.

The existing code throw not implemented exception for the WriteJson method.

https://github.com/NuGet/NuGet.Client/pull/5872/files/f14e6f769612dbaf2e04eb9a702e7b3dcbe8b52b#diff-4e6ff8097dc206e72c42f58c234a65f2875f468c5bd9593c630a4cade52caf0aL49

martinrrm · 2024-09-10T19:43:22Z

New benchmarks, I moved some things, so object initialization is not considered in the benchmarks. With these new changes looks like performance is better 🥳

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
ConverterWithThatUsesLoadJson	15.660 us	0.0328 us	0.0274 us	0.7019	0.0305	17.55 KB
RemovedPackageDependencyGroupConverter	8.631 us	0.0611 us	0.0542 us	0.3204	-	8 KB

kartheekp-ms · 2024-09-11T01:07:41Z

Please consider updating the PR description to reflect the current state of the proposed code changes.

kartheekp-ms · 2024-09-11T01:27:19Z

test/NuGet.Core.Tests/NuGet.Packaging.Test/PackageDependencyGroupTests.cs

+            // Act
+            using var stringReader = new StringReader(PackageRegistrationDependencyGroupsJson_NoTargetFramework);
+            using var jsonReader = new JsonTextReader(stringReader);
+            jsonReader.Read();


Please help me understand why we have to invoke the Read method in the tests.

It is because the first token is null and doing the Read() moves the iterator to the json object, I'm not 100% sure why this happens but looks like we are doing it in other places.

src/NuGet.Core/NuGet.Packaging/Core/PackageDependencyGroup.cs

zivkan

just small nitpicks, or nice to haves. No important comments.

zivkan · 2024-09-11T00:40:31Z

src/NuGet.Core/NuGet.Packaging/Core/PackageDependencyGroup.cs

+        [JsonConstructor]
+        private PackageDependencyGroup(NuGetFramework targetFramework)


The properties are get only, and they're not even auto-props. If Newtonsoft.Json is using reflection to set the value directly on the backing store, I don't understand how it can determine what the backing field name is. 🤷

zivkan · 2024-09-11T00:44:23Z

src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs

+    {
+        public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
+        {
+            serializer.Serialize(writer, value.ToString());


Honestly, I think my preference would be to throw on write, like the old converter did. If anything needs to convert a NuGetFramework to a string in the future, then we can either investigate that "parameterized" converter, or create one converter per string format.

But I can also live with using ToString.

test/NuGet.Core.Tests/NuGet.Packaging.Test/PackageDependencyGroupTests.cs

src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs

martinrrm requested a review from a team as a code owner June 26, 2024 03:55

jgonz120 reviewed Jun 26, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Converters/PackageDependencyGroupConverter.cs Outdated Show resolved Hide resolved

martinrrm requested a review from jgonz120 June 27, 2024 00:02

kartheekp-ms previously approved these changes Jun 27, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Converters/PackageDependencyGroupConverter.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Protocol/Converters/PackageDependencyGroupConverter.cs Outdated Show resolved Hide resolved

martinrrm requested a review from kartheekp-ms June 27, 2024 20:21

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 8, 2024

jeffkl reviewed Jul 10, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Converters/PackageDependencyGroupConverter.cs Outdated Show resolved Hide resolved

dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 10, 2024

zivkan reviewed Jul 11, 2024

View reviewed changes

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 19, 2024

martinrrm removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 19, 2024

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jul 26, 2024

dotnet-policy-service bot closed this Aug 2, 2024

martinrrm reopened this Aug 12, 2024

dotnet-policy-service bot removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 12, 2024

martinrrm requested review from zivkan and jeffkl August 12, 2024 18:51

martinrrm added 7 commits August 12, 2024 11:59

Avoid using LoadJson and iterate by token

b628223

remove unnecesary imports

5299ace

fix token iteration

f050e96

remove comment

92a5e08

Add converter to PackageDependency

1cd48e3

Read as string

5a2349d

Delete Benchmarks/Program.cs

9556342

kartheekp-ms reviewed Aug 20, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Converters/PackageDependencyGroupConverter.cs Outdated Show resolved Hide resolved

deleted converter

64bb8b3

martinrrm requested a review from kartheekp-ms August 21, 2024 17:40

fixed tests

9e99c32

dotnet-policy-service bot added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 30, 2024

martinrrm removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 30, 2024

martinrrm added 3 commits September 5, 2024 16:29

update test to confirm that comments are ignored

9fa0425

Removed converter

fbfce3e

Fix null constructor for target framework

f14e6f7

zivkan reviewed Sep 10, 2024

View reviewed changes

PR comments

78918c9

martinrrm requested a review from zivkan September 10, 2024 20:57

martinrrm changed the title ~~Avoid using LoadJson and iterate by token~~ Remove PackageDependencyGroupConverter Sep 10, 2024

kartheekp-ms reviewed Sep 11, 2024

View reviewed changes

src/NuGet.Core/NuGet.Packaging/Core/PackageDependencyGroup.cs Show resolved Hide resolved

zivkan previously approved these changes Sep 11, 2024

View reviewed changes

kartheekp-ms reviewed Sep 11, 2024

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Converters/NuGetFrameworkConverter.cs Outdated Show resolved Hide resolved

jgonz120 previously approved these changes Sep 11, 2024

View reviewed changes

pr comments

ce23ff7

martinrrm dismissed stale reviews from jgonz120 and zivkan via ce23ff7 September 11, 2024 21:40

martinrrm requested review from kartheekp-ms, zivkan and jgonz120 September 11, 2024 23:28

kartheekp-ms approved these changes Sep 12, 2024

View reviewed changes

martinrrm merged commit fef6562 into dev Sep 12, 2024
28 checks passed

martinrrm deleted the dev-martinrrm-perf-packagedependencygroup-converter branch September 12, 2024 22:10

zivkan mentioned this pull request Oct 22, 2024

VersionRangeConverter should treat empty string same as null #6111

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove PackageDependencyGroupConverter #5872

Remove PackageDependencyGroupConverter #5872

martinrrm commented Jun 26, 2024 •

edited

Loading

martinrrm commented Jun 26, 2024

kartheekp-ms commented Jun 27, 2024

martinrrm commented Jun 27, 2024

zivkan commented Jul 1, 2024

martinrrm commented Aug 12, 2024 •

edited

Loading

zivkan left a comment

zivkan Sep 10, 2024

martinrrm Sep 10, 2024

zivkan Sep 11, 2024

zivkan Sep 10, 2024

martinrrm Sep 10, 2024

zivkan Sep 11, 2024

kartheekp-ms Sep 11, 2024 •

edited

Loading

martinrrm commented Sep 10, 2024 •

edited

Loading

kartheekp-ms commented Sep 11, 2024

kartheekp-ms Sep 11, 2024

martinrrm Sep 12, 2024

zivkan left a comment

zivkan Sep 11, 2024

zivkan Sep 11, 2024

		[JsonConstructor]
		private PackageDependencyGroup(NuGetFramework targetFramework)

Remove PackageDependencyGroupConverter #5872

Remove PackageDependencyGroupConverter #5872

Conversation

martinrrm commented Jun 26, 2024 • edited Loading

Bug

Description

PR Checklist

martinrrm commented Jun 26, 2024

kartheekp-ms commented Jun 27, 2024

martinrrm commented Jun 27, 2024

Before

After

zivkan commented Jul 1, 2024

martinrrm commented Aug 12, 2024 • edited Loading

zivkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

martinrrm commented Sep 10, 2024 • edited Loading

kartheekp-ms commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinrrm commented Jun 26, 2024 •

edited

Loading

martinrrm commented Aug 12, 2024 •

edited

Loading

kartheekp-ms Sep 11, 2024 •

edited

Loading

martinrrm commented Sep 10, 2024 •

edited

Loading