Profiled improvments by silahian · Pull Request #64 · visualHFT/VisualHFT

silahian · 2025-12-13T21:22:04Z

No description provided.

Copilot

Pull request overview

This PR implements performance optimizations identified through profiling, focusing on reducing unnecessary string formatting operations and improving data flow efficiency.

Key Changes

Refactored value formatting from eager (at data generation) to lazy evaluation (at UI rendering time)
Introduced state-based indicators (HasData, HasError, IsStale) to replace hardcoded formatted strings
Added L2/L3 orderbook mode detection in OrderToTradeRatioStudy for dual-mode operation
Extracted magic strings and format constants to improve maintainability

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 19 comments.

Show a summary per file

File	Description
VisualHFT.Commons/Model/BaseStudyModel.cs	Core model refactoring: replaced `ValueFormatted` string property with state flags and optional `CustomFormatter` delegate for lazy formatting
ViewModel/vmTile.cs	Moved formatting logic to UI layer via new `GetDisplayValue` method; enhanced disposal logic for multi-study scenarios
VisualHFT.Commons/PluginManager/BasePluginStudy.cs	Updated error/stale state handling to use new state flags instead of formatted strings
VisualHFT.Plugins/Studies.VPIN/VPINStudy.cs	Extracted color and format constants; switched to `Format` property
VisualHFT.Plugins/Studies.OTT_Ratio/OrderToTradeRatioStudy.cs	Major enhancement: added L2/L3 orderbook mode detection with comprehensive documentation; removed trade event subscription (potential issue); switched to `Format` property
VisualHFT.Plugins/Studies.MarketResilience/MarketResilienceStudy.cs	Switched to `Format` property for consistency
VisualHFT.Plugins/Studies.MarketResilience/MarketResilienceBiasStudy.cs	Implemented `CustomFormatter` delegate pattern for directional bias indicators
VisualHFT.Plugins/Studies.LOBImbalance/LOBImbalanceStudy.cs	Switched to `Format` property for consistency
VisualHFT.Commons/Model/OrderBook.cs	Simplified `ComputeDeltaAgainst` locking mechanism (potential concurrency issue); expanded pattern matching syntax in `UpdateSnapshot` for clarity

Comments suppressed due to low confidence (14)

VisualHFT.Commons/Model/OrderBook.cs:11

Class 'OrderBook' implements 'ICloneable'.

    public partial class OrderBook : ICloneable, IResettable, IDisposable

VisualHFT.Commons/Model/OrderBook.cs:851

Equality checks on floating point values can yield unexpected results.

            return QuantizeToDp(size, dp) == 0.0;

VisualHFT.Commons/Model/OrderBook.cs:768

Equality checks on floating point values can yield unexpected results.

            if (string.IsNullOrEmpty(item.EntryID) && (!item.Price.HasValue || item.Price.Value == 0))

VisualHFT.Commons/Model/OrderBook.cs:629

Condition is always true because of access to property HasValue.

                var _list = (item.IsBid.HasValue && item.IsBid.Value ? _data.Bids : _data.Asks);

VisualHFT.Commons/PluginManager/BasePluginStudy.cs:150

Condition is always false because of access to parameter forceStartRegardlessStatus.

                if (!forceStartRegardlessStatus && Status == ePluginStatus.STOPPED_FAILED) //means that a fata error occurred, and user's attention is needed.

ViewModel/vmTile.cs:188

These 'if' statements can be combined.

                if (_localModel.ValueColor != null)
                {
                    if (_valueColor == null || _valueColor.ToString() != _localModel.ValueColor)
                        ValueColor = new SolidColorBrush((Color)ColorConverter.ConvertFromString(_localModel.ValueColor));
                }

ViewModel/vmTile.cs:36

Field '_study' can be 'readonly'.

        private IStudy _study;

VisualHFT.Commons/Model/OrderBook.cs:14

Field 'lobMetrics' can be 'readonly'.

        private OrderFlowAnalysis lobMetrics = new OrderFlowAnalysis();

VisualHFT.Commons/PluginManager/BasePluginStudy.cs:24

Field '_reconnectionSemaphore' can be 'readonly'.

        private SemaphoreSlim _reconnectionSemaphore = new SemaphoreSlim(1, 1);

VisualHFT.Plugins/Studies.MarketResilience/MarketResilienceBiasStudy.cs:25

Field '_QUEUE' can be 'readonly'.

        private HelperCustomQueue<OrderBookSnapshot> _QUEUE;

VisualHFT.Plugins/Studies.MarketResilience/MarketResilienceStudy.cs:27

Field '_QUEUE' can be 'readonly'.

        private HelperCustomQueue<OrderBookSnapshot> _QUEUE;

VisualHFT.Commons/Model/OrderBook.cs:127

Both branches of this 'if' statement return - consider using '?' to express intent better.

                    if (MaxDepth > 0 && FilterBidAskByMaxDepth)
                        return _data.Asks.Take(MaxDepth);
                    else
                        return _data.Asks;

VisualHFT.Commons/Model/OrderBook.cs:144

Both branches of this 'if' statement return - consider using '?' to express intent better.

                    if (MaxDepth > 0 && FilterBidAskByMaxDepth)
                        return _data.Bids.Take(MaxDepth);
                    else
                        return _data.Bids;

VisualHFT.Commons/Model/OrderBook.cs:678

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.

            if (item.IsBid.Value)
            {
                willNewItemFallOut = listCount > this.MaxDepth && item.Price < list.Min(x => x.Price);
            }
            else
            {
                willNewItemFallOut = listCount > this.MaxDepth && item.Price > list.Max(x => x.Price);
            }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-13T21:38:27Z

+            };
+        }
+
+        // Then in DoCalculationAndSend():


The comment "// Then in DoCalculationAndSend():" appears to be leftover from refactoring or code organization notes. This comment doesn't provide meaningful documentation and should be removed as it doesn't add value to understanding the code.

Suggested change

// Then in DoCalculationAndSend():

Copilot · 2025-12-13T21:38:27Z

+                            {
+                                try
+                                {
+                                    study.StopAsync();


Calling StopAsync without awaiting in the disposal path can lead to incomplete cleanup and potential resource leaks. The async operation may not complete before disposal continues. Consider using "study.StopAsync().GetAwaiter().GetResult()" or making the Dispose pattern properly async, or document why fire-and-forget is acceptable here.

Suggested change

study.StopAsync();

study.StopAsync().GetAwaiter().GetResult();

Copilot · 2025-12-13T21:38:28Z

+            if (!_localModel.HasData && !_localModel.HasError && !_localModel.IsStale)
                _localModel.Tooltip = "Waiting for data...";
-            else if (!string.IsNullOrEmpty(_localModel.Tooltip))
+            else if (!string.IsNullOrEmpty(e.Tooltip))
                _localModel.Tooltip = e.Tooltip;
            else
                _localModel.Tooltip = null;


The tooltip assignment (lines 161-166) occurs outside the lock after copyFrom has completed, while the lock on line 149 protects the copyFrom operation. This creates a race condition where _localModel properties (HasData, HasError, IsStale) could be modified by another thread between the unlock and the tooltip check. Consider moving the tooltip logic inside the lock or using a local copy of the relevant state.

Copilot · 2025-12-13T21:38:28Z

            HelperOrderBook.Instance.Subscribe(LIMITORDERBOOK_OnDataReceived);
-            HelperTrade.Instance.Subscribe(TRADES_OnDataReceived);



The subscription to HelperTrade.Instance has been removed from StartAsync, but the TRADES_OnDataReceived method still exists and appears functional (lines 209-218). If L3 mode relies on trade events from HelperMarketDataStats (as documented in line 31), this trade subscription removal may be intentional. However, if trade counting is still needed in L2 mode, this could break the trade counting functionality. Please verify this is intentional and that trade events are being captured through an alternative mechanism.

Copilot · 2025-12-13T21:38:28Z

        private bool _isFirstL2Call = true;

+        // L3/L2 Mode Detection Variables
+        private volatile bool _IsCurrentLOB_L3 = false;


The variable name "_IsCurrentLOB_L3" uses PascalCase for a private field, which violates C# naming conventions. Private fields should use camelCase (e.g., "_isCurrentLobL3"). This is inconsistent with other private fields in the class like "_isFirstL2Call".

Suggested change

private volatile bool _IsCurrentLOB_L3 = false;

private volatile bool _isCurrentLobL3 = false;

Copilot · 2025-12-13T21:38:31Z

+            {
+                var n = newSide[j++];
+                double np = n.Price.GetValueOrDefault();
+                if (np != 0.0) Emit(isBid, np, n.Size.GetValueOrDefault(), emit);


Equality checks on floating point values can yield unexpected results.

Copilot · 2025-12-13T21:38:31Z

+                            HasError = isError,
+                            IsStale = !isError,
                            Timestamp = HelperTimeProvider.Now,
                            Tooltip = "Provider status: " + newStatus.ToString(),


Redundant call to 'ToString' on a String object.

Copilot · 2025-12-13T21:38:31Z

@@ -38,6 +106,11 @@ public class OrderToTradeRatioStudy : BasePluginStudy
        private long _floorNum = 1; // Default floor; configurable if needed


Field '_floorNum' can be 'readonly'.

Suggested change

private long _floorNum = 1; // Default floor; configurable if needed

private readonly long _floorNum = 1; // Default floor; configurable if needed

Copilot · 2025-12-13T21:38:31Z

-        private long _orderEvents = 0;  
+        private long _orderEvents = 0;
        private long _tradeCount = 0;
        private object _lock = new object();


Field '_lock' can be 'readonly'.

Suggested change

private object _lock = new object();

private readonly object _lock = new object();

Copilot · 2025-12-13T21:38:32Z

+        // L3/L2 Mode Detection Variables
+        private volatile bool _IsCurrentLOB_L3 = false;
+        private DateTime _startedCheckLOB_L3 = DateTime.MinValue;
+        private TimeSpan _LOB3_Identification_Timespan = TimeSpan.FromSeconds(10);


Field '_LOB3_Identification_Timespan' can be 'readonly'.

Suggested change

private TimeSpan _LOB3_Identification_Timespan = TimeSpan.FromSeconds(10);

private readonly TimeSpan _LOB3_Identification_Timespan = TimeSpan.FromSeconds(10);

Ariel Silahian added 2 commits December 13, 2025 16:03

snapshot processing capabilities in the orderbook class

c457a2d

Improved string handling (formating) in studies

3cf5e97

Copilot AI review requested due to automatic review settings December 13, 2025 21:22

silahian merged commit 3111d99 into master Dec 13, 2025
2 checks passed

silahian deleted the profiled-improvments branch December 13, 2025 21:22

Copilot started reviewing on behalf of silahian December 13, 2025 21:22 View session

Copilot AI reviewed Dec 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Profiled improvments#64

Profiled improvments#64
silahian merged 2 commits intomasterfrom
profiled-improvments

silahian commented Dec 13, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	study.StopAsync();
	study.StopAsync().GetAwaiter().GetResult();

		HelperOrderBook.Instance.Subscribe(LIMITORDERBOOK_OnDataReceived);
		HelperTrade.Instance.Subscribe(TRADES_OnDataReceived);

	private volatile bool _IsCurrentLOB_L3 = false;
	private volatile bool _isCurrentLobL3 = false;

		@@ -38,6 +106,11 @@ public class OrderToTradeRatioStudy : BasePluginStudy
		private long _floorNum = 1; // Default floor; configurable if needed

	private long _floorNum = 1; // Default floor; configurable if needed
	private readonly long _floorNum = 1; // Default floor; configurable if needed

	private object _lock = new object();
	private readonly object _lock = new object();

	private TimeSpan _LOB3_Identification_Timespan = TimeSpan.FromSeconds(10);
	private readonly TimeSpan _LOB3_Identification_Timespan = TimeSpan.FromSeconds(10);

Conversation

silahian commented Dec 13, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants