Replace JObjects in PackageMetadataResourceV3 with strong type for better performance, lower memory consumption #3462

erdembayar · 2020-06-18T18:29:32Z

Bug

Last working version:
How are we preventing it in future:

Fix

Details:
Using VS nuget GUI to consolodate different versions of a package crashes the instance of VS. Occurs in 2017, 2019 and 2019 int preview.
One of main problem was PM UI creates giant Jobject and millions of Nuget related objects and ballooning memory.
I am replacing this strong type instead of JObject.

Testing/Validation

Tests Added: Yes
Reason for not adding tests:
Validation: Private unit test.

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

davidfowl · 2020-06-19T02:25:50Z

Where are the before and after benchmarks?

donnie-msft

Nice! Few questions/comments..

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

donnie-msft · 2020-06-19T03:20:08Z

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

-                    .Select(ParseMetadata)
-                    .Select(m => metadataCache.GetObject(m))
-                    .ToArray();
+            var range = VersionRange.All; // This value preset for Consolidate UI.


Are you sure this method is specific to Consolidate? I think several UI codepaths come here.

plus potentially any customer who has a PackageReference to NuGet.Protocol in their own app.

I changed wording. Actually it's from "Update UI" too.

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

src/NuGet.Core/NuGet.Protocol/Utility/MetadataReferenceCache.cs

nkolev92 · 2020-06-19T18:46:41Z

@erdembayar Can you provide some benchmarks please.

Ensure you profile with different sizes of registration pages.

erdembayar · 2020-06-19T20:43:27Z

Where are the before and after benchmarks?

I didn't do any benchmarking yet. But finally I was able to see Consolidate page of this giant solution, I guess that is very good sign of improvement. There are 62 packages need consolidation.

zivkan

If this PR is the only change needed for NuGet/Home#8352, then ok. If that issue is going to need other changes, then please create a new issue specific to the changes in this PR, and link that issue instead.

src/NuGet.Core/NuGet.Protocol/Extensions/VersionRangeExtensions.cs

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

zivkan · 2020-06-19T17:57:13Z

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

-                    .Select(ParseMetadata)
-                    .Select(m => metadataCache.GetObject(m))
-                    .ToArray();
+            var range = VersionRange.All; // This value preset for Consolidate UI.


plus potentially any customer who has a PackageReference to NuGet.Protocol in their own app.

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

zivkan · 2020-06-19T21:51:53Z

Also, this PR's title suggests that all JObjects thoughout the entire codebase were replaced. Please change the title so it's obvious that it's specific to the protocol registration resources.

erdembayar · 2020-06-19T22:35:23Z

Also, this PR's title suggests that all JObjects thoughout the entire codebase were replaced. Please change the title so it's obvious that it's specific to the protocol registration resources.

Ok. I changed PR name.

zivkan · 2020-06-20T00:32:32Z

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

+                {
+                    catalogEntry.ReportAbuseUrl = _reportAbuseResource?.GetReportAbuseUrl(catalogEntry.PackageId, catalogEntry.Version);
+                    catalogEntry.PackageDetailsUrl = _packageDetailsUriResource?.GetUri(catalogEntry.PackageId, catalogEntry.Version);
+                    metadataCache.GetObject(catalogEntry);


The return value of GetObject isn't used. Can we stop calling it and get rid of the changes related to reflection?

Actually I checked it what this thing is doing, it do have benefit. It depopulate same string contents because of string is true immutable type. All same content strings point to same memory address, I did manually checked it, it works for string type, so I decided to improve it. It's hard to explain here, but I can talk to you on Teams.

I'm not sure how this is helping. This is trying to dedup the full object. I don't see the NuGetVersion and Strings being deduped at all.

This caching method was already here before my change so I don't expect any drastic changes, I only added little improvement on reflection part of cache so it would work little faster otherwise it's same thing. Also it only works string property with both getter and setter. I don't think this one decrease number of NuGetVersions, only decrease overall memory size reserved by string properties of instances passed through this cache.
The moment this one runs we already passed Json part and have end result shouldn't see any changes from previously.

I don't see how the strings are getting deduped.
I ran Get on the nuget.packagemanagement package and the cache was empty at the end.

@nkolev92 what do you mean by cache exactly? For example for below case nothing happens, because a and b are already pointing same object reference:
var a = "aaa";
var b = "aaa";

var res = Object.ReferenceEquals(a, b);
Console.WriteLine(res); //Expect true

But below case you would expect deduplication if you use this cache tool, because they're not same object reference.
var a = "aaa";
var b = "aa";
var c = b+ "a";
var res = Object.ReferenceEquals(a, c);
Console.WriteLine(res); //expect False

…overhead and space.

zivkan · 2020-06-24T16:59:56Z

src/NuGet.Core/NuGet.Protocol/Utility/MetadataReferenceCache.cs

+                Type stringPropertyType = typeof(string);
+                MethodInfo method = _metadataReferenceCacheType.GetTypeInfo()
+                        .DeclaredMethods.FirstOrDefault(
+                            m =>
+                                m.Name == CachableTypesMap[stringPropertyType] &&
+                                m.GetParameters().Select(p => p.ParameterType).SequenceEqual(new Type[] { stringPropertyType }));
+                _stringMethodsCache.Add(_metadataReferenceCacheType, method);


This doesn't look right to me. It's looking for methods defined on the MetadataReferenceCache class, rather than generic type T's class. Is that a mistake?

I didn't add this logic, it it was there actually before my change, I just moved to top for multiple use.

zivkan · 2020-06-24T17:16:46Z

src/NuGet.Core/NuGet.Protocol/Utility/MetadataReferenceCache.cs

@@ -17,6 +17,8 @@ public class MetadataReferenceCache
        private readonly Dictionary<string, string> _stringCache = new Dictionary<string, string>(StringComparer.Ordinal);
        private readonly Dictionary<Type, PropertyInfo[]> _propertyCache = new Dictionary<Type, PropertyInfo[]>();
        private readonly Dictionary<string, NuGetVersion> _versionCache = new Dictionary<string, NuGetVersion>(StringComparer.Ordinal);
+        private readonly Dictionary<Type, MethodInfo> _stringMethodsCache = new Dictionary<Type, MethodInfo>();
+        private readonly Type _metadataReferenceCacheType = typeof(MetadataReferenceCache);


This class is MetadataReferenceCache. It seems very strange to me why you'd need to use reflection for this class to access itself.

I didn't add this logic, it it was there actually before my change, I just moved to top for multiple use.

erdembayar · 2020-06-25T02:57:14Z

@nkolev92 @zivkan @davidfowl
I ran benchmarking with Newtosoft.Json package.
It looks runtime is 60% of previous implementation and memory is almost half of previous implementation. You can find my code used for this from here.
To run it:

Compile in Release mode,
Open console cmd. Close VS and don't do anything while running test, it skews test.
go to NuGet.Client\test\Benchmarking\Nuget.Protocol.Benchmarking\bin\Release\netcoreapp3.1
dotnet Nuget.Protocol.Benchmarking.dll
Wait for result.

…Performance

zivkan · 2020-06-25T18:58:39Z

Any ideas why the GC stats are not showing? I can't believe that 3MB of allocations didn't trigger any GC. What TFM did you benchmark with? Do different TFMs have different results (I know we care primarily about Visual Studio, but I'm curious if .NET Core has different perf characteristics).

edit: And as Nikolche pointed out below, it would be good to benchmark small, medium, large json payload sizes to see if there's much difference.

nkolev92 · 2020-06-25T19:00:32Z

Consider using "fake" to benchmark. It contains lots of versions, the true extreme case :)

nkolev92

A few minor suggestions.

Please do the benchmark with different sizes of registration pages for completeness.

nkolev92 · 2020-06-26T02:17:58Z

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

+        /// <param name="log"></param>
+        /// <param name="token"></param>
+        /// <returns>Package meta data.</returns>
+        /// <remarks>The inlined entries are potentially going away soon</remarks>


Best way to determine it by following the references that VS shows you.

I'm not surprised that VS only only goes through the "all" codepath.

test/NuGet.Core.Tests/NuGet.Protocol.Tests/Utility/MetadataReferenceCacheTestUtility.cs

src/NuGet.Core/NuGet.Protocol/Extensions/VersionRangeExtensions.cs

nkolev92 · 2020-06-26T02:36:05Z

Updated the body of the issue to close NuGet/Home#9719 instead of NuGet/Home#8352 because that one is really an epic.

erdembayar · 2020-06-26T03:04:46Z

Any ideas why the GC stats are not showing? I can't believe that 3MB of allocations didn't trigger any GC. What TFM did you benchmark with? Do different TFMs have different results (I know we care primarily about Visual Studio, but I'm curious if .NET Core has different perf characteristics).

edit: And as Nikolche pointed out below, it would be good to benchmark small, medium, large json payload sizes to see if there's much difference. Almost 2 times improvement across all load sizes.

First one I am targeting 3.1 Dotnet core.
Here S=small ("_" package), M=medium("Newtonsoft"), L=large("Fake") loads.

It looks Large = "Fake" has 27 small parts that is making it slow overall, still new implementation is faster than before.

Here is same things for Dot.net 4.8, somehow this one shows result in milliseconds instead of microseconds. Also improvement here is more drastic than .net core for large load, but almost nothing on small loads.
New implementation is almost 2-80 times faster than on large loads, also consume less memory. Not sure why we gain 80 times faster performance during one of benchmark profiling.

…Performance

zivkan

It would be nice to one day refactor NuGet.Protocol to make things easier to read, but who knows if we'll ever get around to it.

Perf improvements look fantastic. Great work! 👍

src/NuGet.Core/NuGet.Protocol/Model/RegistrationIndexResult.cs

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs

erdembayar commented Jun 19, 2020

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs Outdated Show resolved Hide resolved

erdembayar marked this pull request as ready for review June 19, 2020 02:24

erdembayar requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, heng-liu, kartheekp-ms, nkolev92, rrelyea, zivkan and zkat as code owners June 19, 2020 02:24

donnie-msft reviewed Jun 19, 2020

View reviewed changes

zivkan reviewed Jun 19, 2020

View reviewed changes

erdembayar changed the title ~~Replace JObjects with strong type for better performance, lower memory consumption~~ Replace JObjects in PackageMetadataResourceV3 with strong type for better performance, lower memory consumption Jun 19, 2020

zivkan reviewed Jun 20, 2020

View reviewed changes

erdembayar added 8 commits June 22, 2020 13:06

No need dictionary, same thing can be with HashSet which takes lower …

b991d88

…overhead and space.

Replacing slugish JsonObject with strong type.

e5e9622

Revert back unneccessary change.

d491fb5

Add descriptions for new methods added.

00efe67

Add back MetadataReferenceCache cache just in case.

9ab130d

Replace get single package version method too.

d6629c3

Minor code readability change.

597b0b7

Improve MetadataReferenceCache implementation.

598592d

zivkan reviewed Jun 24, 2020

View reviewed changes

erdembayar added 3 commits June 24, 2020 10:40

Address code review by Nkolche

8436bdd

Fix space

c5ab81e

Address more code review comments.

09c188b

Address comment by Andy.

44779ac

erdembayar force-pushed the dev-eryondon-ReplaceJObjectsInmemoryForBetterPerformance branch from f7632f4 to 44779ac Compare June 25, 2020 03:17

erdembayar added 2 commits June 24, 2020 20:21

Merge branch 'dev' into dev-eryondon-ReplaceJObjectsInmemoryForBetter…

17ce59a

…Performance

Merge branch 'dev' into dev-eryondon-ReplaceJObjectsInmemoryForBetter…

082f07f

…Performance

erdembayar mentioned this pull request Jun 25, 2020

OutOfMemoryException when using Package Manager UI NuGet/Home#8352

Closed

nkolev92 approved these changes Jun 26, 2020

View reviewed changes

erdembayar added 2 commits June 25, 2020 20:18

Address more comments by Nkolche.

1158fb1

Merge branch 'dev' into dev-eryondon-ReplaceJObjectsInmemoryForBetter…

ca867ed

…Performance

erdembayar requested a review from zivkan June 26, 2020 16:40

zivkan approved these changes Jun 26, 2020

View reviewed changes

src/NuGet.Core/NuGet.Protocol/Model/RegistrationIndexResult.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Protocol/Resources/PackageMetadataResourceV3.cs Outdated Show resolved Hide resolved

erdembayar requested a review from zivkan June 26, 2020 20:46

Address new comment by Andy.

a2e28fd

erdembayar force-pushed the dev-eryondon-ReplaceJObjectsInmemoryForBetterPerformance branch from ea16b21 to a2e28fd Compare June 26, 2020 21:06

erdembayar added 2 commits June 26, 2020 18:19

Address another comment by Andy for ValueTuple.

84eed9c

Forgot to delete file RegistrationIndexResult.cs which no longer need.

ddad073

erdembayar merged commit 7532d57 into dev Jun 27, 2020

erdembayar deleted the dev-eryondon-ReplaceJObjectsInmemoryForBetterPerformance branch June 27, 2020 16:20

This was referenced Apr 5, 2024

NRE When Package Metadata CatalogPage Leaf returns 404 NuGet/Home#13377

Open

Fix NRE and Disallow Missing Registration Page Leaves #5741

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace JObjects in PackageMetadataResourceV3 with strong type for better performance, lower memory consumption #3462

Replace JObjects in PackageMetadataResourceV3 with strong type for better performance, lower memory consumption #3462

erdembayar commented Jun 18, 2020 •

edited by nkolev92

Loading

davidfowl commented Jun 19, 2020

donnie-msft left a comment

donnie-msft Jun 19, 2020

zivkan Jun 19, 2020

erdembayar Jun 19, 2020

nkolev92 commented Jun 19, 2020

erdembayar commented Jun 19, 2020 •

edited

Loading

zivkan left a comment

zivkan Jun 19, 2020

zivkan commented Jun 19, 2020

erdembayar commented Jun 19, 2020

zivkan Jun 20, 2020

erdembayar Jun 20, 2020 •

edited

Loading

nkolev92 Jun 24, 2020

erdembayar Jun 24, 2020 •

edited

Loading

nkolev92 Jun 24, 2020

erdembayar Jun 24, 2020 •

edited by nkolev92

Loading

zivkan Jun 24, 2020

erdembayar Jun 24, 2020 •

edited

Loading

zivkan Jun 24, 2020

erdembayar Jun 24, 2020 •

edited

Loading

erdembayar commented Jun 25, 2020 •

edited

Loading

zivkan commented Jun 25, 2020 •

edited

Loading

nkolev92 commented Jun 25, 2020

nkolev92 left a comment

nkolev92 Jun 26, 2020

nkolev92 commented Jun 26, 2020

erdembayar commented Jun 26, 2020 •

edited

Loading

zivkan left a comment

Replace JObjects in PackageMetadataResourceV3 with strong type for better performance, lower memory consumption #3462

Replace JObjects in PackageMetadataResourceV3 with strong type for better performance, lower memory consumption #3462

Conversation

erdembayar commented Jun 18, 2020 • edited by nkolev92 Loading

Bug

Fix

Testing/Validation

davidfowl commented Jun 19, 2020

donnie-msft 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

nkolev92 commented Jun 19, 2020

erdembayar commented Jun 19, 2020 • edited Loading

zivkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan commented Jun 19, 2020

erdembayar commented Jun 19, 2020

Choose a reason for hiding this comment

erdembayar Jun 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Jun 24, 2020 • edited by nkolev92 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erdembayar Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

erdembayar commented Jun 25, 2020 • edited Loading

zivkan commented Jun 25, 2020 • edited Loading

nkolev92 commented Jun 25, 2020

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 commented Jun 26, 2020

erdembayar commented Jun 26, 2020 • edited Loading

zivkan left a comment

Choose a reason for hiding this comment

erdembayar commented Jun 18, 2020 •

edited by nkolev92

Loading

erdembayar commented Jun 19, 2020 •

edited

Loading

erdembayar Jun 20, 2020 •

edited

Loading

erdembayar Jun 24, 2020 •

edited

Loading

erdembayar Jun 24, 2020 •

edited by nkolev92

Loading

erdembayar Jun 24, 2020 •

edited

Loading

erdembayar Jun 24, 2020 •

edited

Loading

erdembayar commented Jun 25, 2020 •

edited

Loading

zivkan commented Jun 25, 2020 •

edited

Loading

erdembayar commented Jun 26, 2020 •

edited

Loading