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.

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

Cache trimmed keys for @ and $ #216

Merged
merged 4 commits into from
Mar 10, 2023
Merged

Cache trimmed keys for @ and $ #216

merged 4 commits into from
Mar 10, 2023

Conversation

sungam3r
Copy link
Contributor

@sungam3r sungam3r commented Mar 6, 2023

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
Copy link
Member

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
Copy link
Contributor Author

sungam3r commented Mar 9, 2023

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

# Conflicts:
#	src/Serilog.Extensions.Logging/Extensions/Logging/SerilogLogger.cs
#	src/Serilog.Extensions.Logging/Extensions/Logging/SerilogLoggerScope.cs
@sungam3r
Copy link
Contributor Author

sungam3r commented Mar 9, 2023

Done

@nblumhardt nblumhardt merged commit 01b0d10 into serilog:dev Mar 10, 2023
@nblumhardt
Copy link
Member

Looks great 👍

@sungam3r sungam3r deleted the cache branch March 10, 2023 08:23
@nblumhardt nblumhardt mentioned this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants