Thanks for your PR, @gfoidl. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

The generic method looks prettier! Can this issue be fixed in runtime? https://github.com/dotnet/runtime/blob/cd7e4cacce3b47a2560bc9ec746f0825a43f99d7/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LoggerFactoryExtensions.cs

The generic method looks prettier! Can this issue be fixed in runtime? https://github.com/dotnet/runtime/blob/cd7e4cacce3b47a2560bc9ec746f0825a43f99d7/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LoggerFactoryExtensions.cs

dotnet/runtime#48473 😢

I don't suppose you have any numbers indicating how many instantiations this saves? The change looks unobjectionable and is probably acceptable as a speculative improvement, but my intuition is that it doesn't actually help that much in most cases.

cc @captainsafia for MVC and @BrennanConroy for SignalR

I don't suppose you have any numbers indicating how many instantiations this saves?

I can't tell you how many instantiations it saves, but I just did a quick comparison with NLog provider by creating 10,000,000 instances of ILogger and ILogger<Program> and storing them into two separate arrays. I averaged run times of 3 program runs and the results are 1,016 ms for cached ILogger .vs. 1,882 ms for uncached ILogger<T>.

PrivateMemorySize64 was 100.44 MB for cached .vs. 330.52 MB for uncached.

Of course that's a contrived example, but it shows that both performance and memory are being wasted when using generic ILogger<T> (which is what most people use both in libraries and with dependency injection). I'll let the experts chime in and tell us how much performance and memory is wasted in real code, I am sure they have a way of testing it.

I was curious what the perf difference would be, so I wrote a quick benchmark and ran it.

The TL;DR is that it's ~20% better in time and memory to use CreateLogger(Type) over CreateLogger<T>().

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Diagnosers;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;

namespace DotNetBenchmarks;

[MemoryDiagnoser]
public class ILoggerFactoryBenchmarks
{
    private static readonly ILoggerFactory Factory = NullLoggerFactory.Instance;

    [Benchmark(Baseline = true)]
    public ILogger CreateLogger_Generic()
    {
        return Factory.CreateLogger<MyClass>();
    }

    [Benchmark]
    public ILogger CreateLogger_Type()
    {
        return Factory.CreateLogger(typeof(MyClass));
    }

    private sealed class MyClass
    {
    }
}

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22621.3007/22H2/2022Update/SunValley2)
12th Gen Intel Core i7-1270P, 1 CPU, 16 logical and 12 physical cores
.NET SDK 8.0.101
  [Host]     : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2

Change looks sensible. #50184 was the first PR to introduce this modification to the codebase.

@captainsafia It would have been even better if CreateLogger<T> was fixed by adding caching so that every project can benefit instead of everyone having to stop using CreateLogger<T> to avoid the performance hit.

/azp run

Thanks @gfoidl and sorry for the delay in review.

/azp run