-
Notifications
You must be signed in to change notification settings - Fork 954
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
[Server] [Performance] Reduce locking in CustomNodeManager2 #2895
base: master
Are you sure you want to change the base?
[Server] [Performance] Reduce locking in CustomNodeManager2 #2895
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2895 +/- ##
==========================================
- Coverage 55.43% 55.40% -0.04%
==========================================
Files 352 352
Lines 67602 67561 -41
Branches 13849 13830 -19
==========================================
- Hits 37477 37433 -44
- Misses 26021 26028 +7
+ Partials 4104 4100 -4 ☔ View full report in Codecov by Sentry. |
lets chat about this, I have some ideas... |
…GetManagerHandle threadsafe
Test Issue: Failed TestComponentCacheAsync [4 s] Stack Trace: |
@@ -281,39 +228,31 @@ public void OnReportEvent(ISystemContext context, NodeState node, IFilterTarget | |||
/// <param name="changes">The mask indicating what changes have occurred.</param> | |||
public void OnMonitoredNodeChanged(ISystemContext context, NodeState node, NodeStateChangeMasks changes) | |||
{ | |||
lock (NodeManager.Lock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to enshure Data Changes reach each monitored Item in the right order i would sugesst a semaphore only for this method. No need to lock the entire node manager to enshure this behaviour (the node changes reach this method anyway)
|
||
for (int ii = 0; ii < DataChangeMonitoredItems.Count; ii++) | ||
foreach (MonitoredItem monitoredItem in DataChangeMonitoredItems?.Values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Parallel.ForEach
Proposed changes
Types of changes
Checklist
Further comments
ToDo
Optimized Methods:
CustomNodeManger:
Parallel Tests:
To Decide: