Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource #6300

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource #6300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a74bb6
Reduce memory allocations in HttpFileSystembasedFindPackageByIdResource
zivkan Mar 5, 2025
f7673b9
don't forget to sort
zivkan Mar 5, 2025
0896820
Changes
zivkan Mar 6, 2025
f7673eb
Update field name to be more accurate
zivkan Mar 6, 2025
d948b1a
minor improvements
zivkan Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/NuGet.Core/NuGet.Protocol/GlobalSuppressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,6 @@
[assembly: SuppressMessage("Build", "CA1303:Method 'Task<DownloadResourceResult> GetDownloadResultUtility.GetDownloadResultAsync(HttpSource client, PackageIdentity identity, Uri uri, PackageDownloadContext downloadContext, string globalPackagesFolder, ILogger logger, CancellationToken token)' passes a literal string as parameter 'message' of a call to 'InvalidOperationException.InvalidOperationException(string message)'. Retrieve the following string(s) from a resource table instead: \"Reached an unexpected point in the code\".", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.GetDownloadResultUtility.GetDownloadResultAsync(NuGet.Protocol.HttpSource,NuGet.Packaging.Core.PackageIdentity,System.Uri,NuGet.Protocol.Core.Types.PackageDownloadContext,System.String,NuGet.Common.ILogger,System.Threading.CancellationToken)~System.Threading.Tasks.Task{NuGet.Protocol.Core.Types.DownloadResourceResult}")]
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'Task HttpCacheUtility.CreateCacheFileAsync(HttpCacheResult result, HttpResponseMessage response, Action<Stream> ensureValidContents, CancellationToken cancellationToken)', validate parameter 'result' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpCacheUtility.CreateCacheFileAsync(NuGet.Protocol.HttpCacheResult,System.Net.Http.HttpResponseMessage,System.Action{System.IO.Stream},System.Threading.CancellationToken)~System.Threading.Tasks.Task")]
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'HttpCacheResult HttpCacheUtility.InitializeHttpCacheResult(string httpCacheDirectory, Uri sourceUri, string cacheKey, HttpSourceCacheContext context)', validate parameter 'context' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpCacheUtility.InitializeHttpCacheResult(System.String,System.Uri,System.String,NuGet.Protocol.Core.Types.HttpSourceCacheContext)~NuGet.Protocol.HttpCacheResult")]
[assembly: SuppressMessage("Build", "CA1308:In method 'BuildModel', replace the call to 'ToLowerInvariant' with 'ToUpperInvariant'.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.BuildModel(System.String,System.String,System.String)~NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.PackageInfo")]
[assembly: SuppressMessage("Build", "CA1822:Member BuildModel does not access instance data and can be marked as static (Shared in VisualBasic)", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.BuildModel(System.String,System.String,System.String)~NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.PackageInfo")]
[assembly: SuppressMessage("Build", "CA1308:In method 'FindPackagesByIdAsync', replace the call to 'ToLowerInvariant' with 'ToUpperInvariant'.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.FindPackagesByIdAsync(System.String,NuGet.Protocol.Core.Types.SourceCacheContext,NuGet.Common.ILogger,System.Threading.CancellationToken)~System.Threading.Tasks.Task{System.Collections.Generic.SortedDictionary{NuGet.Versioning.NuGetVersion,NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResource.PackageInfo}}")]
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'Task<Tuple<bool, INuGetResource>> HttpFileSystemBasedFindPackageByIdResourceProvider.TryCreate(SourceRepository sourceRepository, CancellationToken token)', validate parameter 'sourceRepository' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpFileSystemBasedFindPackageByIdResourceProvider.TryCreate(NuGet.Protocol.Core.Types.SourceRepository,System.Threading.CancellationToken)~System.Threading.Tasks.Task{System.Tuple{System.Boolean,NuGet.Protocol.Core.Types.INuGetResource}}")]
[assembly: SuppressMessage("Build", "CA1062:In externally visible method 'Task<Tuple<bool, INuGetResource>> HttpHandlerResourceV3Provider.TryCreate(SourceRepository source, CancellationToken token)', validate parameter 'source' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpHandlerResourceV3Provider.TryCreate(NuGet.Protocol.Core.Types.SourceRepository,System.Threading.CancellationToken)~System.Threading.Tasks.Task{System.Tuple{System.Boolean,NuGet.Protocol.Core.Types.INuGetResource}}")]
[assembly: SuppressMessage("Build", "CA1031:Modify 'GetAsync' to catch a more specific allowed exception type, or rethrow the exception.", Justification = "<Pending>", Scope = "member", Target = "~M:NuGet.Protocol.HttpSource.GetAsync``1(NuGet.Protocol.HttpSourceCachedRequest,System.Func{NuGet.Protocol.HttpSourceResult,System.Threading.Tasks.Task{``0}},NuGet.Common.ILogger,System.Threading.CancellationToken)~System.Threading.Tasks.Task{``0}")]
Expand Down
170 changes: 81 additions & 89 deletions ...uGet.Core/NuGet.Protocol/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
using System.Globalization;
using System.IO;
using System.Linq;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Threading;
using System.Threading.Tasks;
using NuGet.Common;
Expand All @@ -34,9 +36,10 @@ public class HttpFileSystemBasedFindPackageByIdResource : FindPackageByIdResourc
private const int DefaultMaxRetries = 3;
private int _maxRetries;
private readonly HttpSource _httpSource;
private readonly ConcurrentDictionary<string, AsyncLazy<SortedDictionary<NuGetVersion, PackageInfo>>> _packageInfoCache =
new ConcurrentDictionary<string, AsyncLazy<SortedDictionary<NuGetVersion, PackageInfo>>>(StringComparer.OrdinalIgnoreCase);
private readonly ConcurrentDictionary<string, AsyncLazy<HashSet<NuGetVersion>>> _packageVersionsCache =
new ConcurrentDictionary<string, AsyncLazy<HashSet<NuGetVersion>>>(StringComparer.OrdinalIgnoreCase);
private readonly IReadOnlyList<Uri> _baseUris;
private string _chosenBaseUri;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private readonly FindPackagesByIdNupkgDownloader _nupkgDownloader;
private readonly EnhancedHttpRetryHelper _enhancedHttpRetryHelper;

Expand Down Expand Up @@ -127,9 +130,9 @@ public override async Task<IEnumerable<NuGetVersion>> GetAllVersionsAsync(
{
cancellationToken.ThrowIfCancellationRequested();

var packageInfos = await EnsurePackagesAsync(id, cacheContext, logger, cancellationToken);
var packageVersions = await GetAvailablePackageVersionsAsync(id, cacheContext, logger, cancellationToken);

return packageInfos.Keys;
return packageVersions;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
finally
{
Expand Down Expand Up @@ -192,22 +195,24 @@ public override async Task<FindPackageByIdDependencyInfo> GetDependencyInfoAsync
{
cancellationToken.ThrowIfCancellationRequested();

var packageInfos = await EnsurePackagesAsync(id, cacheContext, logger, cancellationToken);
var packageVersions = await GetAvailablePackageVersionsAsync(id, cacheContext, logger, cancellationToken);

PackageInfo packageInfo;
if (packageInfos.TryGetValue(version, out packageInfo))
if (!packageVersions.Contains(version))
{
var reader = await _nupkgDownloader.GetNuspecReaderFromNupkgAsync(
packageInfo.Identity,
packageInfo.ContentUri,
cacheContext,
logger,
cancellationToken);

return GetDependencyInfo(reader);
// package version not found
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consider omitting comment - the code seems clear

return null;
}

return null;
var nupkgUrl = GetNupkgUrl(id, version);

var reader = await _nupkgDownloader.GetNuspecReaderFromNupkgAsync(
new PackageIdentity(id, version),
nupkgUrl,
cacheContext,
logger,
cancellationToken);

return GetDependencyInfo(reader);
}
finally
{
Expand Down Expand Up @@ -278,21 +283,23 @@ public override async Task<bool> CopyNupkgToStreamAsync(
{
cancellationToken.ThrowIfCancellationRequested();

var packageInfos = await EnsurePackagesAsync(id, cacheContext, logger, cancellationToken);
var packageVersions = await GetAvailablePackageVersionsAsync(id, cacheContext, logger, cancellationToken);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Please use an explicit type so readers immediately know this is a hashset.


PackageInfo packageInfo;
if (packageInfos.TryGetValue(version, out packageInfo))
if (!packageVersions.Contains(version))
{
return await _nupkgDownloader.CopyNupkgToStreamAsync(
packageInfo.Identity,
packageInfo.ContentUri,
destination,
cacheContext,
logger,
cancellationToken);
return false;
}

return false;
var packageIdentity = new PackageIdentity(id, version);
var nupkgUrl = GetNupkgUrl(id, version);

return await _nupkgDownloader.CopyNupkgToStreamAsync(
packageIdentity,
nupkgUrl,
destination,
cacheContext,
logger,
cancellationToken);
}
finally
{
Expand Down Expand Up @@ -342,15 +349,13 @@ public override async Task<IPackageDownloader> GetPackageDownloaderAsync(

cancellationToken.ThrowIfCancellationRequested();

var packageInfos = await EnsurePackagesAsync(packageIdentity.Id, cacheContext, logger, cancellationToken);

PackageInfo packageInfo;
if (packageInfos.TryGetValue(packageIdentity.Version, out packageInfo))
var packageVersions = await GetAvailablePackageVersionsAsync(packageIdentity.Id, cacheContext, logger, cancellationToken);
if (!packageVersions.Contains(packageIdentity.Version))
{
return new RemotePackageArchiveDownloader(_httpSource.PackageSource, this, packageInfo.Identity, cacheContext, logger);
return null;
}

return null;
return new RemotePackageArchiveDownloader(_httpSource.PackageSource, this, packageIdentity, cacheContext, logger);
}

/// <summary>
Expand Down Expand Up @@ -403,9 +408,9 @@ public override async Task<bool> DoesPackageExistAsync(
{
cancellationToken.ThrowIfCancellationRequested();

var packageInfos = await EnsurePackagesAsync(id, cacheContext, logger, cancellationToken);
var packageVersions = await GetAvailablePackageVersionsAsync(id, cacheContext, logger, cancellationToken);

return packageInfos.TryGetValue(version, out var packageInfo);
return packageVersions.Contains(version);
}
finally
{
Expand All @@ -418,16 +423,16 @@ public override async Task<bool> DoesPackageExistAsync(
}
}

private async Task<SortedDictionary<NuGetVersion, PackageInfo>> EnsurePackagesAsync(
private async ValueTask<HashSet<NuGetVersion>> GetAvailablePackageVersionsAsync(
string id,
SourceCacheContext cacheContext,
ILogger logger,
CancellationToken cancellationToken)
{
AsyncLazy<SortedDictionary<NuGetVersion, PackageInfo>> result = null;
AsyncLazy<HashSet<NuGetVersion>> result = null;

Func<string, AsyncLazy<SortedDictionary<NuGetVersion, PackageInfo>>> findPackages =
(keyId) => new AsyncLazy<SortedDictionary<NuGetVersion, PackageInfo>>(
Func<string, AsyncLazy<HashSet<NuGetVersion>>> findPackages =
(keyId) => new AsyncLazy<HashSet<NuGetVersion>>(
() => FindPackagesByIdAsync(
keyId,
cacheContext,
Expand All @@ -437,18 +442,18 @@ private async Task<SortedDictionary<NuGetVersion, PackageInfo>> EnsurePackagesAs
if (cacheContext.RefreshMemoryCache)
{
// Update the cache
result = _packageInfoCache.AddOrUpdate(id, findPackages, (k, v) => findPackages(id));
result = _packageVersionsCache.AddOrUpdate(id, findPackages, (k, v) => findPackages(id));
}
else
{
// Read the cache if it exists
result = _packageInfoCache.GetOrAdd(id, findPackages);
result = _packageVersionsCache.GetOrAdd(id, findPackages);
}

return await result;
}

private async Task<SortedDictionary<NuGetVersion, PackageInfo>> FindPackagesByIdAsync(
private async Task<HashSet<NuGetVersion>> FindPackagesByIdAsync(
string id,
SourceCacheContext cacheContext,
ILogger logger,
Expand Down Expand Up @@ -480,13 +485,14 @@ private async Task<SortedDictionary<NuGetVersion, PackageInfo>> FindPackagesById
},
async httpSourceResult =>
{
var result = new SortedDictionary<NuGetVersion, PackageInfo>();
HashSet<NuGetVersion> result = null;

if (httpSourceResult.Status == HttpSourceResultStatus.OpenedFromDisk)
{
try
{
result = await ConsumeFlatContainerIndexAsync(httpSourceResult.Stream, id, baseUri, cancellationToken);
_chosenBaseUri = baseUri;
}
catch
{
Expand All @@ -498,6 +504,7 @@ private async Task<SortedDictionary<NuGetVersion, PackageInfo>> FindPackagesById
else if (httpSourceResult.Status == HttpSourceResultStatus.OpenedFromNetwork)
{
result = await ConsumeFlatContainerIndexAsync(httpSourceResult.Stream, id, baseUri, cancellationToken);
_chosenBaseUri = baseUri;
}

return result;
Expand Down Expand Up @@ -540,65 +547,50 @@ ex.InnerException is IOException &&
return null;
}

private async Task<SortedDictionary<NuGetVersion, PackageInfo>> ConsumeFlatContainerIndexAsync(Stream stream, string id, string baseUri, CancellationToken token)
private static async Task<HashSet<NuGetVersion>> ConsumeFlatContainerIndexAsync(Stream stream, string id, string baseUri, CancellationToken token)
{
var doc = await stream.AsJObjectAsync(token);
var json = await JsonSerializer.DeserializeAsync<FlatContainerVersionList>(stream, cancellationToken: token);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: just name it cancellationToken?


var streamResults = new SortedDictionary<NuGetVersion, PackageInfo>();
var result =
#if NETSTANDARD
new HashSet<NuGetVersion>();
#else
new HashSet<NuGetVersion>(capacity: json.Versions.Count);
#endif

var versions = doc["versions"];
if (versions == null)
foreach (var versionString in json.Versions)
{
return streamResults;
NuGetVersion parsedVersion = NuGetVersion.Parse(versionString);
result.Add(parsedVersion);
}

foreach (var packageInfo in versions
.Select(x => BuildModel(baseUri, id, x.ToString()))
.Where(x => x != null))
{
if (!streamResults.ContainsKey(packageInfo.Identity.Version))
{
streamResults.Add(packageInfo.Identity.Version, packageInfo);
}
}

return streamResults;
return result;
}

private PackageInfo BuildModel(string baseUri, string id, string version)
private string GetNupkgUrl(string id, NuGetVersion version)
{
var parsedVersion = NuGetVersion.Parse(version);
var normalizedVersionString = parsedVersion.ToNormalizedString();
var normalizedVersionString = version.ToNormalizedString().ToLowerInvariant();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable given we're calling ToLower on the Id as well.
However, can the tests be updated to match this? If I'm not mistaken, this test would fail after your change if the test data were uppercase (but it's not, so it won't fail now). Just making 1 uppercase character and amending this assertion would cover that?
https://github.com/NuGet/NuGet.Client/blob/d948b1a55a189466ccb627c7f754226d9f43505f/test/NuGet.Core.Tests/NuGet.Protocol.Tests/RemoteRepositories/HttpFileSystemBasedFindPackageByIdResourceTests.cs#L328C27-L328C28

string idInLowerCase = id.ToLowerInvariant();

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

builder.Append(baseUri);
builder.Append(idInLowerCase);
builder.Append('/');
builder.Append(normalizedVersionString);
builder.Append('/');
builder.Append(idInLowerCase);
builder.Append('.');
builder.Append(normalizedVersionString);
builder.Append(".nupkg");

string contentUri = StringBuilderPool.Shared.ToStringAndReturn(builder);

return new PackageInfo
{
Identity = new PackageIdentity(id, parsedVersion),
ContentUri = contentUri,
};
var baseUri = _chosenBaseUri ?? _baseUris[0].OriginalString;

string contentUri = string.Concat(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string contentUri = string.Concat(

Would an interpolated string make this easier to read?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Contributor

@ToddGrun ToddGrun Mar 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

baseUri,
idInLowerCase,
"/",
normalizedVersionString,
"/",
idInLowerCase,
".",
normalizedVersionString,
".nupkg");

return contentUri;
}

private class PackageInfo
record FlatContainerVersionList
{
public PackageIdentity Identity { get; set; }

public string Path { get; set; }

public string ContentUri { get; set; }
[JsonPropertyName("versions")]
public List<string> Versions { get; set; }
}
}
}
Loading