Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource #6300

zivkan · 2025-03-05T05:36:39Z

Bug

Description

Following @ToddGrun's PR: #6265, in particular this comment thread: #6265 (comment)

I noticed that the code is creating one PackageInfo and PackageIdentity and ContentUri for every package version the server knows about. However, in a restore, typically only a single package version is needed. So, by creating those PackageIdentity and url strings only when needed, we can remove a whole lot of additional allocations over what the other PR did.

In addition to removing PackageInfo and associated types, I moved the JSON parsing to System.Text.Json instead of Newtonsoft.Json (which used JObject, no less). So, that provides us with just most of Todd's PR memory improvements as well.

PR Checklist

Meaningful title, helpful description and a linked NuGet/Home issue
~~Added tests~~ code was refactored, no behavior changes
~~Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.~~ N/A

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

ToddGrun · 2025-03-05T15:04:20Z

I like this PR, it was a good find that the PackageInfo didn't really need to be in this cache.

ToddGrun · 2025-03-05T16:01:47Z

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

            string idInLowerCase = id.ToLowerInvariant();
+            var baseUri = _baseUris[retry % _baseUris.Count].OriginalString;

            var builder = StringBuilderPool.Shared.Rent(256);


var builder = StringBuilderPool.Shared.Rent(256);

When you debug this, do you see many of the url exceeding 256 and thus not working well with the pool?

While talking to Jeff, he pointed out that the string isn't dynamic in any way (no if (something) builder.Add), so we can use string.Concat instead.

It's still probably preferred over potentially removing items from the stringbuilder pool, but I think netfx will end up allocating the array on the heap to hold the individual strings.

GetAllVersionsAsync probably doesn't need to be sorted. This lets us use a HashSet for quicker lookups in all other APIs. Don't change the baseUri used to generate nupkg download URIs String.Concat instead of StringBuilder

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

zivkan · 2025-03-06T00:40:10Z

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs


-                return packageInfos.Keys;
+                return packageVersions;


Since _packageVersionsCache now contains HashSet rather than SortedDictionary, this means the public API GetAllVersionsAsync may no longer be returning a sorted list of versions. So, changing this risks breaking behaviour.

However, FindPackageById is only used by restore, within NuGet.Client. PM UI, and anything that cares about unlisted package status, does not use this API. Restore handles this list not being sorted, and all tests appear to be passing, so I think the biggest risk are apps using our NuGet.Protocol package.

zivkan · 2025-03-06T00:41:47Z

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

        private readonly IReadOnlyList<Uri> _baseUris;
+        private string _chosenBaseUri;


I don't like this, I would have liked to make it round-robin load balance across all the base URIs that the server provided in their service index. But that would be a change in behavior which might risk breaking customers. So, we re-use the base uri that the version list was obtained from, just like the old code.

ToddGrun · 2025-03-06T00:45:15Z

I'd love to hear about what measurements you ended up taking and how big of an improvement this is

ToddGrun · 2025-03-06T16:39:25Z

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

-            };
+            var baseUri = _chosenBaseUri ?? _baseUris[0].OriginalString;
+
+            string contentUri = string.Concat(


string contentUri = string.Concat(

Would an interpolated string make this easier to read?

string contentUri = $"{baseUri}{idInLowerCase}/{normalizedVersionString}/{idInLowerCase}.{normalizedVersionString}.nupkg";

Isn't string.concat supposed to be faster than interpolation or format?

I was playing around on sharplab and they look pretty similar (at least on netfx) and although different on core, I'm not really sure that one is better than the other.

I agree that interpolation would be more readable, but this seems like a hotpath and a reasonably long string, so my understanding is Concat is more performant.

For the longest time, I understood concat or + for strings to be sloppy and bad performance. But a few years ago I learned that was somehow the opposite of reality!

Therefore, if my understanding is still up to date, I'd say leave it as is or compromise for readability with:
baseUri + idInLowerCase + "/" + normalizedVersionString + "/" + idInLowerCase ...

I'm fine with whatever is decided here, but I do think the perf of interpolated strings is better than you guys think:

A simple benchmark comparing String.Concat and interpolation actually indicates interpolation performance is about the same on net472 and actually better than String.Concat in net9.

net472

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated

Concat 150.5 us 1.56 us 1.46 us 54.9316 - - 282 KB

ConcatIncludeLoopCounter 233.9 us 3.23 us 3.03 us 63.2324 - - 325 KB

Interpolated 147.3 us 0.87 us 0.81 us 54.9316 - - 282 KB

InterpolatedIncludeLoopCounter 162.0 us 1.14 us 1.01 us 56.3965 - - 289 KB

net9.0

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated

Concat 43.86 us 0.642 us 0.536 us 28.2593 - - 289 KB

ConcatIncludeLoopCounter 73.05 us 1.286 us 1.140 us 44.1895 - - 451 KB

Interpolated 38.67 us 0.354 us 0.331 us 19.1040 - - 195 KB

InterpolatedIncludeLoopCounter 42.38 us 0.433 us 0.384 us 19.1040 - - 195 KB

zivkan · 2025-03-06T23:17:55Z

I'd love to hear about what measurements you ended up taking and how big of an improvement this is

From: #6265 (comment)

branch	GC stats total allocs	GC Heap Alloc Ignore Free Stacks ConsumeFlatContainer
dev	252 MB	5,512,529
this PR	246 MB	2,139,213
my PR	244 MB	669,655

But keep in mind in the comment, from the other PR, I had the caveat that I amit I pretty much don't know what I'm doing.

Here's a PerfView diff of the dev branch and this PR branch heap alloc view:

Finally, I thought I'd try to use BenchmarkDotNet's MemoryAnalyzer. Here's a patch to the NuGet.Client repo with my benchmark code:
benchmark.zip

However, when I run it, I get these results:

Method	Mean	Error	StdDev	Ratio	Gen0	Gen1	Allocated	Alloc Ratio
Change	2.739 ms	0.0161 ms	0.0126 ms	1.00	328.1250	23.4375	1.99 MB	1.00
Original	2.656 ms	0.0246 ms	0.0205 ms	0.97	316.4063	23.4375	1.91 MB	0.96

So, this benchmark results make it look like my change has more allocations and is slower than the original, which frankly doesn't make any sense to me whatsoever.

What started as reviewing another PR turned into trying to implement further improvements, and while code changes took probably less than an hour, I've now been working probably 2 full days on collecting perf measurements. I was behind my assigned work for this sprint before I started any of this, so I just can't justify any more effort. If this PR is "acceptable" as-is, or with minor changes, I can do that. But I don't have the capacity to investigate any further why these perf measurements are so out of whack.

ToddGrun · 2025-03-07T02:22:20Z

I'm very happy with this PR (for what that's worth). I definitely wasn't trying to steal a bunch of anyone on the nuget's team time (which is why I made the original PR myself). I went ahead and did some measurements based on this branch, and will share below. My measurements were based on the following steps: "git clean -xdff" in roslyn repo, delete the global nuget cache, open VS, start profiler, and open roslyn.sln, wait for the status bar to indicate everything is done, stop profiler.

I took two profiles each with and without this change and averaged the allocations/times. Filtering under FindPackagesByIdAsync, this change showed an allocation improve from 155 MB to 25 MB and CPU samples improve from 21000 ms to 3850 ms.

In reply to: 2705152207

ToddGrun · 2025-03-07T03:50:28Z

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

        {
-            var doc = await stream.AsJObjectAsync(token);
+            var json = await JsonSerializer.DeserializeAsync<FlatContainerVersionList>(stream, cancellationToken: token);


JsonSerializer.DeserializeAsync

This shows up as a very small amount of the remaining allocations, so there is definitely no need to pursue the other PR.

zivkan · 2025-03-07T21:59:56Z

While falling asleep last night, I realized that my benchmark was reusing a single SourceRepository instance, which means the same HttpFileSystemBasedFindPackageByIdResource instance, which means the warmup would warm the cache, and then the benchmark was only testing the cache-hit code path! It also explains why my change was slower and using more memory, because the old code pre-cached the content URL and PackageIdentity for every package version, whereas my change allocates those types on demand.

Fixing the benchmark, I get different results:

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen0	Gen1	Allocated	Alloc Ratio
Change	206.4 ms	4.11 ms	5.20 ms	1.00	0.03	1000.0000	-	20.67 MB	1.00
Original	201.5 ms	4.02 ms	7.25 ms	0.98	0.04	1333.3333	666.6667	24.77 MB	1.20

I'm still very surprised the changed code is slower, wall clock time. But at least we're now correctly measuring the memory allocation improvements.

donnie-msft

Nice work, and I look forward to seeing how perf improves as a result of this PR!

Not sure if I feel reassured or worried about test coverage given that no tests needed to be changed here. I did suggest 1 string related test change.

donnie-msft · 2025-03-08T01:51:43Z

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

-            };
+            var baseUri = _chosenBaseUri ?? _baseUris[0].OriginalString;
+
+            string contentUri = string.Concat(


I agree that interpolation would be more readable, but this seems like a hotpath and a reasonably long string, so my understanding is Concat is more performant.

For the longest time, I understood concat or + for strings to be sloppy and bad performance. But a few years ago I learned that was somehow the opposite of reality!

Therefore, if my understanding is still up to date, I'd say leave it as is or compromise for readability with:
baseUri + idInLowerCase + "/" + normalizedVersionString + "/" + idInLowerCase ...

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

donnie-msft · 2025-03-08T02:00:37Z

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

        {
-            var doc = await stream.AsJObjectAsync(token);
+            var json = await JsonSerializer.DeserializeAsync<FlatContainerVersionList>(stream, cancellationToken: token);


src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

ToddGrun · 2025-03-11T15:30:20Z

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs

+                versionString = null;
+                return false;
+            }
+            versionString = version.ToNormalizedString().ToLowerInvariant();


version.ToNormalizedString().ToLowerInvariant();

If the !NETSTANDARD case can get away without needing the ToLowerInvariant, couldn't the else clause versionString assignment be changed to:

versionString = packageVersions[version].ToNormalizedString();

Unfortunately, we can't remove the ToLowerInvariant(), for the reason described here: #6300 (comment). Perhaps I need to improve the comment at the top of the NETSTANDARD block

I'm pretty sure I'm being super dense. I definitely believe that the ToLowerInvariant can be needed, I just don't understand why the !NETSTANDARD block wouldn't need it then.

Just to make sure we're on the same page, I was suggesting that the NETSTANDARD case also use the value from the HashSet, not from the input version.

Unless there's an API I'm not aware of, the only way to get the value out of the hashset on netstandard is to iterate the hashset as if it was a list and stop when the value is equal. HashSet<T> on netstandard doesn't have a TryGetValue API, like the comment above the code says.

But yes, if there is an easy/cheap way to get the value out of the hashset, then I'd have it work the same way as netframework and netcoreapp.

Gotcha, it was very stuck in my mind that there was an indexer on HashSet. Thanks for the clarification!

ToddGrun

Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource

2a74bb6

zivkan mentioned this pull request Mar 5, 2025

Remove some allocations under HttpFileSystemBasedFindPackageByIdResource.ConsumeFlatContainerIndexAsync #6265

Closed

3 tasks

don't forget to sort

f7673b9

ToddGrun reviewed Mar 5, 2025

View reviewed changes

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs Outdated Show resolved Hide resolved

ToddGrun reviewed Mar 5, 2025

View reviewed changes

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs Outdated Show resolved Hide resolved

ToddGrun reviewed Mar 5, 2025

View reviewed changes

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs Outdated Show resolved Hide resolved

ToddGrun reviewed Mar 5, 2025

View reviewed changes

zivkan added 2 commits March 6, 2025 10:33

Changes

0896820

GetAllVersionsAsync probably doesn't need to be sorted. This lets us use a HashSet for quicker lookups in all other APIs. Don't change the baseUri used to generate nupkg download URIs String.Concat instead of StringBuilder

Update field name to be more accurate

f7673eb

ToddGrun reviewed Mar 6, 2025

View reviewed changes

src/NuGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs Outdated Show resolved Hide resolved

zivkan marked this pull request as ready for review March 6, 2025 00:35

zivkan requested a review from a team as a code owner March 6, 2025 00:35

zivkan requested review from jeffkl and donnie-msft March 6, 2025 00:35

zivkan commented Mar 6, 2025

View reviewed changes

ToddGrun reviewed Mar 6, 2025

View reviewed changes

ToddGrun reviewed Mar 7, 2025

View reviewed changes

minor improvements

d948b1a

zivkan requested a review from nkolev92 March 7, 2025 22:02

donnie-msft reviewed Mar 8, 2025

View reviewed changes

zivkan added 2 commits March 10, 2025 20:51

More improvements

e0c8a75

Add null annotations

2fd7771

zivkan force-pushed the dev-zivkan-flatcontainer-versionlist branch from f450122 to 2fd7771 Compare March 10, 2025 10:51

donnie-msft previously approved these changes Mar 10, 2025

View reviewed changes

fix bug

74b4113

zivkan dismissed donnie-msft’s stale review via 74b4113 March 10, 2025 22:42

donnie-msft approved these changes Mar 10, 2025

View reviewed changes

ToddGrun reviewed Mar 11, 2025

View reviewed changes

ToddGrun approved these changes Mar 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource #6300

Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource #6300

zivkan commented Mar 5, 2025 •

edited

Loading

ToddGrun commented Mar 5, 2025

ToddGrun Mar 5, 2025

zivkan Mar 5, 2025

ToddGrun Mar 5, 2025

zivkan Mar 6, 2025

zivkan Mar 6, 2025

ToddGrun commented Mar 6, 2025

ToddGrun Mar 6, 2025

nkolev92 Mar 6, 2025

ToddGrun Mar 7, 2025

donnie-msft Mar 8, 2025

ToddGrun Mar 8, 2025 •

edited

Loading

zivkan commented Mar 6, 2025

ToddGrun commented Mar 7, 2025

ToddGrun Mar 7, 2025

donnie-msft Mar 8, 2025

zivkan commented Mar 7, 2025

donnie-msft left a comment

donnie-msft Mar 8, 2025

donnie-msft Mar 8, 2025

ToddGrun Mar 11, 2025

zivkan Mar 11, 2025

ToddGrun Mar 11, 2025

ToddGrun Mar 11, 2025

zivkan Mar 12, 2025 •

edited

Loading

ToddGrun Mar 12, 2025

ToddGrun left a comment

		private readonly IReadOnlyList<Uri> _baseUris;
		private string _chosenBaseUri;

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
Concat	150.5 us	1.56 us	1.46 us	54.9316	-	-	282 KB
ConcatIncludeLoopCounter	233.9 us	3.23 us	3.03 us	63.2324	-	-	325 KB
Interpolated	147.3 us	0.87 us	0.81 us	54.9316	-	-	282 KB
InterpolatedIncludeLoopCounter	162.0 us	1.14 us	1.01 us	56.3965	-	-	289 KB

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
Concat	43.86 us	0.642 us	0.536 us	28.2593	-	-	289 KB
ConcatIncludeLoopCounter	73.05 us	1.286 us	1.140 us	44.1895	-	-	451 KB
Interpolated	38.67 us	0.354 us	0.331 us	19.1040	-	-	195 KB
InterpolatedIncludeLoopCounter	42.38 us	0.433 us	0.384 us	19.1040	-	-	195 KB

Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource #6300

Are you sure you want to change the base?

Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource #6300

Conversation

zivkan commented Mar 5, 2025 • edited Loading

Bug

Description

PR Checklist

ToddGrun commented Mar 5, 2025

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

ToddGrun commented Mar 6, 2025

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

ToddGrun Mar 8, 2025 • edited Loading

Choose a reason for hiding this comment

zivkan commented Mar 6, 2025

ToddGrun commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan commented Mar 7, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun left a comment

Choose a reason for hiding this comment

zivkan commented Mar 5, 2025 •

edited

Loading

ToddGrun Mar 8, 2025 •

edited

Loading

zivkan Mar 12, 2025 •

edited

Loading