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

[Server] [Performance] Reduce locking in CustomNodeManager2 #2895

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

romanett
Copy link
Contributor

@romanett romanett commented Dec 10, 2024

Proposed changes

  • Make NodeIdDictionary a ConcurrentDictionary and therefore allow to gradually remove locks from many parts of the server
  • Update (m_)PredefinedNodes in CustomNodeManager2 to be used without locks
  • make m_namespaceUris + (m_)NamespaceIndexes in CustomNodeManager2 ReadOnlyCollections and therefore threadsafe
  • make m_componentCache a NodeIdDictionary
  • make m_rootNotifiers a NodeIdDictionary
  • make (m_)MonitoredNodes a NodeIdDictionary (faster compare + concurrentDictionary)
  • make IsNodeInNamespace /IsHandleInNamespace /GetManagerHandle threadsafe
  • make underlying List of MIs in MonitoredNode a ConcurrentDictionary instead of lists, as the implementation before was not efficient

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Test enhancement (non-breaking change to increase test coverage)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc.
  • I have signed the CLA.
  • I ran tests locally with my changes, all passed.
  • I fixed all failing tests in the CI pipelines.
  • I fixed all introduced issues with CodeQL and LGTM.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added necessary documentation (if appropriate).
  • Any dependent changes have been merged and published in downstream modules.

Further comments

ToDo

  • Concurrent tests for RootNotifier & MonitoredNode

Optimized Methods:

  • AlarmNodeManager Call
    CustomNodeManger:
  • SetNamespaces / SetNamespaceIndexes / IsNodeIdInNamespace (make thread safe by using IReadOnlyCollection for m_namespaceUris + (m_)NamespaceIndexes)
  • Find(remove lock)
  • DeleteNode (remove lock, only lock modifying m_rootNotifiers)
  • AddPredefinedNode
  • AddRootNotifier, RemoveRootNotifier (make Lock Free)
  • DeleteAddressSpace (remove lock by copying predefined nodes to temp array)
  • GetManagerHandle remove lock (IsNodeIdInNamespace & m_predefinedNodes are threadsafe)
  • DeleteReference, GetNodeMetadata, GetPermissionMetadata, Browse, TranslateBrowsePath, Call (lock only after validating nodes are in the namespace of the NodeManager)
  • CreateMonitoredItems, ModifyMonitoredItems, DeleteMonitoredItems, SetMonitoringMode (lock only after validating nodes are in the namespace of the NodeManager)
  • LookupNodeInComponentCache, RemoveNodeFromComponentCache, AddNodeToComponentCache (Make these methods work Lock Free by making m_componentCache a NodeIDictionary)
  • SubscribeToEvents (Add missing locks in callers)

Parallel Tests:

  • ComponentCache
  • PredefinedNodesDictionary

To Decide:

  • iterate over Values of Dictionary (snapshot) or iterate KVPs (updated in Realtime, no snapshot)

@romanett romanett requested a review from mregen December 10, 2024 06:18
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 63.06306% with 82 lines in your changes missing coverage. Please review.

Project coverage is 55.40%. Comparing base (0777bbd) to head (c427487).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
...ies/Opc.Ua.Server/Diagnostics/CustomNodeManager.cs 65.02% 49 Missing and 15 partials ⚠️
...braries/Opc.Ua.Server/Diagnostics/MonitoredNode.cs 48.14% 10 Missing and 4 partials ⚠️
...tack/Opc.Ua.Core/Types/BuiltIn/NodeIdDictionary.cs 62.50% 2 Missing and 1 partial ⚠️
...ibraries/Opc.Ua.Client/Session/TraceableSession.cs 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@mregen
Copy link
Contributor

mregen commented Dec 10, 2024

lets chat about this, I have some ideas...

@romanett romanett changed the title [Server] Make predefinedNodes a ConcurrentDictionary [Server] [Performance] Reduce locking in CustomNodeManager2 Dec 14, 2024
@romanett romanett marked this pull request as ready for review December 17, 2024 17:10
@romanett
Copy link
Contributor Author

Test Issue:

Failed TestComponentCacheAsync [4 s]
Error Message:
Assert.That(handleFromCache, Is.Not.Null)
Expected: not null
But was: null

Stack Trace:
at Opc.Ua.Server.Tests.CustomNodeManagerTests.UseComponentCacheAsync(TestableCustomNodeManger2 nodeManager, BaseObjectState baseObject, NodeHandle nodeHandle) in //Tests/Opc.Ua.Server.Tests/CustomNodeManagerTests.cs:line 150
at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
at Opc.Ua.Server.Tests.CustomNodeManagerTests.UseComponentCacheAsync(TestableCustomNodeManger2 nodeManager, BaseObjectState baseObject, NodeHandle nodeHandle)
at Opc.Ua.Server.Tests.CustomNodeManagerTests.<>c__DisplayClass0_0.b__0() in /
/Tests/Opc.Ua.Server.Tests/CustomNodeManagerTests.cs:line 41
at Opc.Ua.Server.Tests.CustomNodeManagerTests.<>c__DisplayClass4_0.<b__0>d.MoveNext() in /_/Tests/Opc.Ua.Server.Tests/CustomNodeManagerTests.cs:line 177

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Parallel.ForEach

@romanett romanett marked this pull request as draft December 23, 2024 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants