serilog / serilog-extensions-logging

Serilog provider for Microsoft.Extensions.Logging
Apache License 2.0
307 stars 97 forks source link

Cache trimmed keys for @ and $ #216

Closed sungam3r closed 1 year ago

sungam3r commented 1 year ago

BenchmarkDotNet=v0.13.5, OS=Windows 11 (10.0.22621.1265/22H2/2022Update/SunValley2)
AMD Ryzen 7 6800H with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET SDK=7.0.201
  [Host]     : .NET 5.0.17 (5.0.1722.21314), X64 RyuJIT AVX2
  DefaultJob : .NET 5.0.17 (5.0.1722.21314), X64 RyuJIT AVX2

before

Method Mean Error StdDev Gen0 Allocated
LogInformation 3.141 μs 0.0555 μs 0.0519 μs 0.1717 1.42 KB
LogInformationScoped 4.421 μs 0.0408 μs 0.0381 μs 0.2899 2.38 KB

after

Method Mean Error StdDev Gen0 Allocated
LogInformation 3.066 μs 0.0348 μs 0.0325 μs 0.1640 1.36 KB
LogInformationScoped 3.948 μs 0.0320 μs 0.0300 μs 0.2823 2.32 KB
nblumhardt commented 1 year ago

It looks like a nice win; normally we view unbounded cache growth as a bug; I can see how in this case it'd generally indicate misuse by a caller, but elsewhere (e.g. MessageTemplateCache) we've hardened up the code against this kind of problem.

(For example, since the log message may be some generated string, it's not impossible to imagine the generated string erroneously including "{$some-guid} and filling the cache with garbage.)

Is there a cheap way to limit the cache size? Perhaps since $ (stringify) is rarely used, we could not do the caching there, and for @ (capture) we could first check the cache size and fall through to the substring operation if we see > 1000 entries?

sungam3r commented 1 year ago

I'd use cache for both cases. Ok with limiting size.

sungam3r commented 1 year ago

Done

nblumhardt commented 1 year ago

Looks great 👍