Skip to content

Avoid redundant SourceRepository allocation on cache hit in CachingSourceProvider#7132

Merged
zivkan merged 1 commit intoNuGet:devfrom
nareshjo:CreateRepository-SourceRepositoryAlloc
Feb 19, 2026
Merged

Avoid redundant SourceRepository allocation on cache hit in CachingSourceProvider#7132
zivkan merged 1 commit intoNuGet:devfrom
nareshjo:CreateRepository-SourceRepositoryAlloc

Conversation

@nareshjo
Copy link
Copy Markdown
Contributor

@nareshjo nareshjo commented Feb 12, 2026

🤖 AI-Generated Pull Request 🤖

This pull request was generated by the VS Perf Rel AI Agent. Please review this AI-generated PR with extra care! For more information, visit our wiki. Please share feedback with TIP Insights


  • Issue: CachingSourceProvider.CreateRepository calls ConcurrentDictionary.GetOrAdd(key, value) where the value is new SourceRepository(source, _resourceProviders, type).

    This eagerly constructs a SourceRepository on every call — even when the key already exists in the cache and the newly-constructed instance is immediately discarded.

    The SourceRepository constructor calls Init()GroupBy()Sort(), allocating multiple List<T>, Dictionary<Type, ...>, and Int32[] instances per construction even if they are already cached.

    This matches the allocation stack flow showing DependencyGraphSpecRequestProvider.GetRequestsFromItemsRestoreArgs.GetEffectiveSourcesCoreCachingSourceProvider.CreateRepositorySourceRepository..ctorSourceRepository.InitSourceRepository.SortList<T>..ctorEnumerableSorter.SortJIT_NewArr1SVR::GCHeap::AllocTypeAllocated!System.Int32[]

nuget.commands.dll!NuGet.Commands.DependencyGraphSpecRequestProvider+<>c__DisplayClass_0.<GetRequestsFromItems>b__
nuget.commands.dll!NuGet.Commands.RestoreArgs.GetEffectiveSourcesCore
nuget.protocol.dll!NuGet.Protocol.CachingSourceProvider.CreateRepository
nuget.protocol.dll!NuGet.Protocol.Core.Types.SourceRepository..ctor
nuget.protocol.dll!NuGet.Protocol.Core.Types.SourceRepository.Init
nuget.protocol.dll!NuGet.Protocol.Core.Types.SourceRepository.Sort
mscorlib.dll!System.Collections.Generic.List`[System.__Canon]..ctor
system.core.dll!System.Linq.EnumerableSorter`[System.__Canon].Sort
clr.dll!JIT_NewArr1
clr.dll!SVR::GCHeap::Alloc
TypeAllocated!System.Int32[]
  • Issue type: Eliminate redundant SourceRepository construction and associated LINQ/sort/collection allocations on cache-hit hot path

  • Proposed fix: Add a TryGetValue fast-path check before GetOrAdd in CachingSourceProvider.CreateRepository.
    On a cache hit (the common case under parallel restore), TryGetValue returns the existing SourceRepository with zero allocations — no constructor call, no closure, no LINQ overhead. The GetOrAdd fallback is only reached on a cache miss, where the SourceRepository construction cost is unavoidable and identical to the original behavior.

    The _cachedSources dictionary is append-only (no removals anywhere in the codebase), so a TryGetValue hit guarantees the key remains present, making the non-atomic TryGetValueGetOrAdd sequence safe.

    Alternatively, if a single dictionary access is preferred, GetOrAdd(key, _ => new SourceRepository(...)) can be used — it trades a small closure allocation (~32 bytes) per call for simpler code, which is still a major improvement over the current eager construction.

Best practices wiki
See related failure in PRISM
ADO work item

@nareshjo nareshjo requested a review from a team as a code owner February 12, 2026 23:53
@nareshjo nareshjo requested review from jebriede and zivkan February 12, 2026 23:53
@dotnet-policy-service dotnet-policy-service bot added the Community PRs created by someone not in the NuGet team label Feb 12, 2026
@nkolev92
Copy link
Copy Markdown
Member

nkolev92 commented Feb 17, 2026

@nareshjo After some discussion, people suggested using the Func overload with a static method to avoid capture class allocations.

cc @zivkan

@zivkan
Copy link
Copy Markdown
Member

zivkan commented Feb 18, 2026

I wanted to double-check my assumption, and yes, passing a func without using an anonymous function wrapper avoids any allocations:

Method source feedType Mean Error StdDev Gen0 Gen1 Allocated
AlwaysAlloc nuget(...)json] [47] HttpV3 31,221.41 ns 371.593 ns 329.408 ns 8.4839 0.6104 53533 B
TryGetThenAdd nuget(...)json] [47] HttpV3 17.55 ns 0.167 ns 0.157 ns - - -
AnonFunc nuget(...)json] [47] HttpV3 24.08 ns 0.152 ns 0.127 ns 0.0166 - 104 B
StaticRealFunc nuget(...)json] [47] HttpV3 18.24 ns 0.083 ns 0.073 ns - - -
source code
using System.Collections.Concurrent;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using NuGet.Configuration;
using NuGet.Protocol;
using NuGet.Protocol.Core.Types;

[MemoryDiagnoser]
public class Program
{
    private static void Main(string[] args)
    {
        BenchmarkRunner.Run<Program>();
    }

    private static PackageSource _nugetOrg = new PackageSource("https://api.nuget.org/v3/index.json", "nuget.org");

    private ConcurrentDictionary<string, SourceRepository> _dictionary = new ConcurrentDictionary<string, SourceRepository>
    {
        [_nugetOrg.Source] = Repository.Factory.GetCoreV3(_nugetOrg.Source),
    };

    private IEnumerable<Lazy<INuGetResourceProvider>> _providers = Repository.Provider.GetCoreV3();

    public IEnumerable<object[]> GetSources()
    {
        yield return [_nugetOrg, FeedType.HttpV3];
    }

    [Benchmark]
    [ArgumentsSource(nameof(GetSources))]
    public SourceRepository AlwaysAlloc(PackageSource source, FeedType feedType)
    {
        return _dictionary.GetOrAdd(source.Source, new SourceRepository(source, _providers, feedType));
    }

    [Benchmark]
    [ArgumentsSource(nameof(GetSources))]
    public SourceRepository TryGetThenAdd(PackageSource source, FeedType feedType)
    {
        if (_dictionary.TryGetValue(source.Source, out var repository))
        {
            return repository;
        }
        return _dictionary.GetOrAdd(source.Source, new SourceRepository(source, _providers, feedType));
    }

    [Benchmark]
    [ArgumentsSource(nameof(GetSources))]
    public SourceRepository AnonFunc(PackageSource source, FeedType feedType)
    {
        return _dictionary.GetOrAdd(source.Source, _ => new SourceRepository(source, _providers, feedType));
    }

    [Benchmark]
    [ArgumentsSource(nameof(GetSources))]
    public SourceRepository StaticRealFunc(PackageSource source, FeedType feedType)
    {
        return _dictionary.GetOrAdd(source.Source, Factory, (_providers, feedType));

        static SourceRepository Factory(string key, (IEnumerable<Lazy<INuGetResourceProvider>>, FeedType) args)
        {
            var (providers, feedType) = args;
            return new SourceRepository(new PackageSource(key), providers, feedType);
        }
    }
}

I don't know if GetOrAdd with a factory method has better perf than TryGet followed by GetOrAdd when it actually needs to add, but it looks like when the value is already in the cache both approaches have pretty much the same perf. But the cache miss scenario should be uncommon (most solutions only have a small number of package sources), so maybe the perf difference really doesn't matter and we can just take this PR as-is.

@nareshjo
Copy link
Copy Markdown
Contributor Author

@nkolev92, let me know if you'd still prefer to switch it to a static method after considering @zivkan's comment, which notes that cache misses should be uncommon, so either approach should be fine. I'm happy to make the change - let me know

@nkolev92
Copy link
Copy Markdown
Member

I'm fine with it as is.
I'll let @zivkan make the call as he had the original feedback.

@zivkan zivkan merged commit 3a2648c into NuGet:dev Feb 19, 2026
17 checks passed
@nareshjo nareshjo deleted the CreateRepository-SourceRepositoryAlloc branch February 19, 2026 04:37
@zivkan zivkan mentioned this pull request Mar 4, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community PRs created by someone not in the NuGet team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants