MicroTrendsLtd / NinjaTrader8

NinjaTrader8 Components Strategies and Trading tools
MIT License
72 stars 18 forks source link

Inclusion of precise 'small in scope' Locks on all accesses to Lists - RE: Issue 2 of 3 #7

Closed jmscraig closed 3 years ago

jmscraig commented 3 years ago

CONTEXT: These updates are a direct response to issues documented in issue number five Titled "Second of Three Issues - Conflicts Concurrent Access to Lists" https://github.com/MicroTrendsLtd/NinjaTrader8/issues/5

MOTIVATION: On fast charts List access conflicts and Deadlocks threatened the viability of using the system. The changes within this Pull-Request implement the most straight-forward and lowest risk response recommended first step in documented in the the issue titled "Conflicts Concurrent Access to Lists" >> "The Fix: I think just implementing fine grain, very small in scope (so fast), unique locks per List will go a long way toward solving issues here."

SCOPE: The Pull-Request includes a wide range of low risk changes providing 'small in scope' Locks for all access to lists in one file AlgoSystemBase.cs

TESTING: 2 hours running 4 charts ( 8 ticks, 6 ticks, 6 ticks and 20 ticks) on afterhours live market data on four different instruments each chart implementing as different Strategy properties configurations as possible. I have not noticed any system errors the NT8 Log, Output tab 1, Output tab 2, or in a scan of the ATS.NT8.xxx files. All Workflow Cases have not yet occurred so some have not yet been tested. Of course more testing and a watchful eye is needed.

NEXT STEPS:
1) All Workflow Cases have not yet occurred so some have not yet been tested. Of course more testing and a watchful eye is needed. 2) If the addition of these locks do not resolve concurrent access concerns with acceptable performance consider migration from Lists to using ConcurrentDictionarys which are twice as slow to add but have much faster, less blocking read-only access performance than locks. 2) Doing something to reduce the scope of the Lock in OnOrderUpdate to not include full execution of the TradeWorkflow Case structure.

MicroTrendsTom commented 3 years ago

ok awesome ty