-
Notifications
You must be signed in to change notification settings - Fork 1k
performance: Reduce allocations (logger factory reuse, check enabled) #3387
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: Reduce allocations (logger factory reuse, check enabled) #3387
Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3387 +/- ##
==========================================
+ Coverage 51.86% 59.95% +8.08%
==========================================
Files 370 374 +4
Lines 78618 78304 -314
Branches 13650 13620 -30
==========================================
+ Hits 40779 46947 +6168
+ Misses 33705 27026 -6679
- Partials 4134 4331 +197 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks @RicoSuter - this is great, one more thing off my plate :-) |
|
@marcschier i think the changes here are really low-risk, no? Reusing the factory instead of creating needs to be checked but seems also safe for me... will dig deeper into more complex improvements when i have time. |
|
@marcschier my personal goal is still to achieve 1.2 mio changes in both directions and on server side <100ms average :-) not even sure whether this is possible with OPC UA :-D |
|
@RicoSuter, one of the changes we need for that to happen is to reduce GC pressure for monitored item notification and DataValue, Variant, and node id allocations on the subscribe/publish paths client side, we have some benchmarks and the improvements are worth it although it will break apis. Goal is before April next year. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@marcschier do you expect that I add unit tests for the changed LoCs? |
|
@RicoSuter - No, most of it was for legacy pieces, and the one you fixed is actively tested already... Sorry took longer to merge the queue is quite full right now... |
|
@marcschier I've run benchmarks again and updated the PR: Client receive processing looks good now (even a bit faster), server side still degraded (4-8%) but this is less a problem because most clients do not write that much. Comparison grid here: Thanks for merging my PR. |
Proposed changes
Some performance optimizations, reducing memory allocations by ~23% while maintaining full API compatibility and thread-safety.
Related Issues
Types of changes
What types of changes does your code introduce?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
1. LoggerFactory Singleton Pattern
File:
Stack/Opc.Ua.Core/Types/Utils/LoggerUtils.cs:95-99Introduced singleton pattern for
TraceLoggerTelemetryto eliminate repeated allocations:Impact: Primary source of 23% allocation reduction
Safety: Thread-safe, no API changes
2. Additional Optimizations
The upstream library also includes:
Subscription.csAll changes maintain backward compatibility.
Benchmark results
Master branch (baseline):
This PR (optimizations):
Client => from 97 mb/s to 74 mb/s, 5ms lower latency on avg
Server => from 505 mb/s to 474 mb/s, 30ms lower latency on avg