-
Notifications
You must be signed in to change notification settings - Fork 794
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
Performance Improvements - CreateLogger()
and Logging Initialization
#2038
base: dev
Are you sure you want to change the base?
Performance Improvements - CreateLogger()
and Logging Initialization
#2038
Conversation
Thanks for sending this and the other five benchmarking PRs, @rafaelsc! Given benchmarks have the same ongoing maintenance costs as other kinds of tests, I think we need to grow the benchmark suite in a fairly conservative manner. I don't think we should merge these currently as it's not clear we'll be able to get benefit from them in the immediate future. We tend to use benchmarks in a style that pairs them with optimizations in a specific area; or, we create temporary benchmarks when making those kinds of changes (e.g. #1944 (comment)). If you're keen to dig into perf, perhaps working on a specific chunk of candidate code (say, JSON formatting, or something similar that hasn't been updated to match modern .NET APIs) and pairing improvements with benchmarks would be a smoother path forward? Appreciate your efforts, and thanks nonetheless! |
Hi @nblumhardt, Years ago I paired many performance optimizations in the code with update/new benchmarks, and you said that has "hard to keep up, and to review it properly" because of the size of the changes. I'm been working in change for ~30% performance improvement and ~5% less memory usage for I can add this changes to this PR and put the Benchmark (Old and new) in the comments. In some weeks. For the others I will need more time to migrate the old improvements and update them. |
Hi @rafaelsc, thanks for the reply! I should probably have been clearer, small changes are good, but ideally we should review and discuss only one or two of them at a time. It's not just the change itself, but the purpose and plan that we can sync up on that way. The If you're keen to tackle anything that takes a lot of time and isn't in the issues list with a discussion already, sending a heads-up to make sure we're on the same page first will help things roll along smoothly. Thanks again for all the PRs, nonetheless! |
…n, internally. Prefer Collection Expression.
Avoid LINQ. Use EmptySink when no sink was set. Prefer Collection Expression.
CreateLogger()
and Logging Initialization
I just update this PR (Title, Description, Changes) with the performance improvement, and I'll be be extreme happy in updating with any "opinionated" changes. My goal is just make Seirlog faster then Microsoft Logging again. But I notice the new Recent changes made on #2058, #2060, #2055, and I will need to look this recent changes another day. |
Recent changes was checked. This is new BenchmarkDotNet=v0.13.5, OS=Windows 11 (10.0.22631.3593)
AMD Ryzen 9 3900XT, 1 CPU, 24 logical and 12 physical cores
.NET SDK=8.0.300
[Host] : .NET 8.0.5 (8.0.524.21615), X64 RyuJIT AVX2
DefaultJob : .NET 8.0.5 (8.0.524.21615), X64 RyuJIT AVX2
And this is the changes added on this PR.
Up to ~35% of improvement and less ~20% allocation, for 4 sinks, filter and properties. |
Changes:
EmptySink
when no sink was set.CreateLogger()
and the Serilog initialization time and memory usage.