From 11366351da475f715d084354ff333805ca461f53 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 1 Dec 2016 18:22:36 +0300 Subject: [PATCH 1/5] Improve a progress pane performance (one thread) 1. All updates left in one thread. (The use of multiple threads was the main problem previous PR #2640) 2. The timer is only used to set up a update flag. (According to my tests calling Datetime.Now() and verifing the time delta creates a much larger delay) 3. The timer interval is 200 ms. I test intervals from 50 ms to 2000 ms. I don't see any differences on the screen with 100 ms and 200 ms. So I chose the more cost-effective option. What do you think about this? 4. Removed unnecessary locking. `WriteProgress` is executed as quickly as possible, as a result the entire script executes significantly faster --- .../host/msh/ConsoleHostUserInterface.cs | 7 +-- .../msh/ConsoleHostUserInterfaceProgress.cs | 63 +++++++++++++++++-- 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs index 81942ff4092..e514105097b 100644 --- a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs +++ b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs @@ -1390,10 +1390,9 @@ public override void WriteProgress(Int64 sourceId, ProgressRecord record) } else { - lock (_instanceLock) - { - HandleIncomingProgressRecord(sourceId, record); - } + // lock (_instanceLock) is moved into HandleIncomingProgressRecord + // to exclude unneeded locks + HandleIncomingProgressRecord(sourceId, record); } } } diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs index 89aa6e91476..a8529dc1af2 100644 --- a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs +++ b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs @@ -7,6 +7,7 @@ using System; using System.Management.Automation; using Dbg = System.Management.Automation.Diagnostics; +using System.Threading; namespace Microsoft.PowerShell @@ -28,10 +29,19 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt // destroy the data structures representing outstanding progress records // take down and destroy the progress display + if (_progPaneUpdateTimer != null) + { + // Stop update a progress pane and destroy timer + _progPaneUpdateTimer.Dispose(); + _progPaneUpdateTimer = null; + } + progPaneUpdateFlag = false; + if (_progPane != null) { Dbg.Assert(_pendingProgress != null, "How can you have a progress pane and no backing data structure?"); + // lock (_instanceLock) is not needed here because lock is done at global level _progPane.Hide(); _progPane = null; } @@ -60,19 +70,60 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt _pendingProgress = new PendingProgress(); } - _pendingProgress.Update(sourceId, record); - if (_progPane == null) { - // This is the first time we've received a progress record. Create a progress pane, then show it. + // This is the first time we've received a progress record + // Create a progress pane + // Set up a update flag + // Create a timer for updating the flag _progPane = new ProgressPane(this); + + if (_progPaneUpdateTimer == null && _progPane != null) + { + // Show a progress pane at the first time we've received a progress record + progPaneUpdateFlag = true; + + // Invoke the timer only once to exclude overlaps + // The timer will be restarted in 'ProgressPaneUpdateTimerElapsed' + _progPaneUpdateTimer = new Timer( new TimerCallback(ProgressPaneUpdateTimerElapsed), null, UpdateTimerThreshold, Timeout.Infinite); + } + } + + if (progPaneUpdateFlag) + { + // Update the progress pane only when the timer set up the flag + // as a result, we do not block WriteProgress and whole script + // and eliminate unnecessary console locks and updates + lock (_instanceLock) + { + _pendingProgress?.Update(sourceId, record); + _progPane?.Show(_pendingProgress); + progPaneUpdateFlag = false; + } } - _progPane.Show(_pendingProgress); } + /// + /// + /// TimerCallback for _progPaneUpdateTimer to update 'progPaneUpdateFlag' and restart the timer + /// + /// + + private + void + ProgressPaneUpdateTimerElapsed(object sender) + { + if (_progPane != null) + { + progPaneUpdateFlag = true; + } + + _progPaneUpdateTimer?.Change(UpdateTimerThreshold, Timeout.Infinite); + } + private void PreWrite() @@ -166,6 +217,10 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt private ProgressPane _progPane = null; private PendingProgress _pendingProgress = null; + // The timer set up 'progPaneUpdateFlag' every 'UpdateTimerThreshold' milliseconds to update 'ProgressPane' + private Timer _progPaneUpdateTimer; + private const int UpdateTimerThreshold = 200; + private bool progPaneUpdateFlag; } } // namespace From 0a494c130b13a434c862db5fd3e884a7eaba97f0 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Mon, 13 Mar 2017 14:53:00 +0300 Subject: [PATCH 2/5] Return the lock to ConsoleHostUserInterface.cs --- .../host/msh/ConsoleHostUserInterface.cs | 7 ++++--- .../host/msh/ConsoleHostUserInterfaceProgress.cs | 16 ++++++---------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs index e514105097b..81942ff4092 100644 --- a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs +++ b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs @@ -1390,9 +1390,10 @@ public override void WriteProgress(Int64 sourceId, ProgressRecord record) } else { - // lock (_instanceLock) is moved into HandleIncomingProgressRecord - // to exclude unneeded locks - HandleIncomingProgressRecord(sourceId, record); + lock (_instanceLock) + { + HandleIncomingProgressRecord(sourceId, record); + } } } } diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs index a8529dc1af2..9b2869a7cd4 100644 --- a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs +++ b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs @@ -90,17 +90,13 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt } } - if (progPaneUpdateFlag) + if (progPaneUpdateFlag || record.RecordType == ProgressRecordType.Completed) { - // Update the progress pane only when the timer set up the flag - // as a result, we do not block WriteProgress and whole script - // and eliminate unnecessary console locks and updates - lock (_instanceLock) - { - _pendingProgress?.Update(sourceId, record); - _progPane?.Show(_pendingProgress); - progPaneUpdateFlag = false; - } + // Update the progress pane only when the timer set up the update flag or WriteProgress is completed. + // As a result, we do not block WriteProgress and whole script and eliminate unnecessary console locks and updates. + _pendingProgress.Update(sourceId, record); + _progPane.Show(_pendingProgress); + progPaneUpdateFlag = false; } } From 1b6d784615bc8dd29d9815c73bb8df7f64ecf041 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 17 Mar 2017 13:07:44 +0300 Subject: [PATCH 3/5] Remove redundent --- .../host/msh/ConsoleHostUserInterfaceProgress.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs index 9b2869a7cd4..9eeb96ed222 100644 --- a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs +++ b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs @@ -35,7 +35,10 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt _progPaneUpdateTimer.Dispose(); _progPaneUpdateTimer = null; } - progPaneUpdateFlag = false; + // We don't reset 'progPaneUpdateFlag = false' here + // because `HandleIncomingProgressRecord` will be init 'progPaneUpdateFlag' in any way. + // (Also the timer callback can still set it to 'true' accidentally) + if (_progPane != null) { @@ -79,7 +82,7 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt _progPane = new ProgressPane(this); - if (_progPaneUpdateTimer == null && _progPane != null) + if (_progPaneUpdateTimer == null) { // Show a progress pane at the first time we've received a progress record progPaneUpdateFlag = true; From 144b28e3d9db58ff866025b2397c78fd45141b72 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 17 Mar 2017 16:05:41 +0300 Subject: [PATCH 4/5] Move to Interlocked.CompareExchange --- .../host/msh/ConsoleHostUserInterfaceProgress.cs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs index 9eeb96ed222..b515d248cae 100644 --- a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs +++ b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs @@ -73,6 +73,8 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt _pendingProgress = new PendingProgress(); } + _pendingProgress.Update(sourceId, record); + if (_progPane == null) { // This is the first time we've received a progress record @@ -85,7 +87,7 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt if (_progPaneUpdateTimer == null) { // Show a progress pane at the first time we've received a progress record - progPaneUpdateFlag = true; + progPaneUpdateFlag = 1; // Invoke the timer only once to exclude overlaps // The timer will be restarted in 'ProgressPaneUpdateTimerElapsed' @@ -93,13 +95,11 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt } } - if (progPaneUpdateFlag || record.RecordType == ProgressRecordType.Completed) + if (Interlocked.CompareExchange(ref progPaneUpdateFlag, 0, 1) == 1 || record.RecordType == ProgressRecordType.Completed) { // Update the progress pane only when the timer set up the update flag or WriteProgress is completed. // As a result, we do not block WriteProgress and whole script and eliminate unnecessary console locks and updates. - _pendingProgress.Update(sourceId, record); _progPane.Show(_pendingProgress); - progPaneUpdateFlag = false; } } @@ -115,10 +115,7 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt void ProgressPaneUpdateTimerElapsed(object sender) { - if (_progPane != null) - { - progPaneUpdateFlag = true; - } + Interlocked.CompareExchange(ref progPaneUpdateFlag, 1, 0); _progPaneUpdateTimer?.Change(UpdateTimerThreshold, Timeout.Infinite); } @@ -219,7 +216,7 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt // The timer set up 'progPaneUpdateFlag' every 'UpdateTimerThreshold' milliseconds to update 'ProgressPane' private Timer _progPaneUpdateTimer; private const int UpdateTimerThreshold = 200; - private bool progPaneUpdateFlag; + private int progPaneUpdateFlag; } } // namespace From 51c8adeba0c30b8763d2728f38abc19de87f852f Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 17 Mar 2017 23:30:50 +0300 Subject: [PATCH 5/5] Correct comments, timer, initializations and lock --- .../msh/ConsoleHostUserInterfaceProgress.cs | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs index b515d248cae..c9e7e761a48 100644 --- a/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs +++ b/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterfaceProgress.cs @@ -29,26 +29,30 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt // destroy the data structures representing outstanding progress records // take down and destroy the progress display - if (_progPaneUpdateTimer != null) + // If we have multiple runspaces on the host then any finished pipeline in any runspace will lead to call 'ResetProgress' + // so we need the lock + lock (_instanceLock) { - // Stop update a progress pane and destroy timer - _progPaneUpdateTimer.Dispose(); - _progPaneUpdateTimer = null; - } - // We don't reset 'progPaneUpdateFlag = false' here - // because `HandleIncomingProgressRecord` will be init 'progPaneUpdateFlag' in any way. - // (Also the timer callback can still set it to 'true' accidentally) - + if (_progPaneUpdateTimer != null) + { + // Stop update a progress pane and destroy timer + _progPaneUpdateTimer.Dispose(); + _progPaneUpdateTimer = null; + } + // We don't set 'progPaneUpdateFlag = 0' here, because: + // 1. According to MSDN, the timer callback can occur after the Dispose() method has been called. + // So we cannot guarantee the flag is truly set to 0. + // 2. When creating a new timer in 'HandleIncomingProgressRecord', we will set the flag to 1 anyway - if (_progPane != null) - { - Dbg.Assert(_pendingProgress != null, "How can you have a progress pane and no backing data structure?"); + if (_progPane != null) + { + Dbg.Assert(_pendingProgress != null, "How can you have a progress pane and no backing data structure?"); - // lock (_instanceLock) is not needed here because lock is done at global level - _progPane.Hide(); - _progPane = null; + _progPane.Hide(); + _progPane = null; + } + _pendingProgress = null; } - _pendingProgress = null; } @@ -89,9 +93,8 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt // Show a progress pane at the first time we've received a progress record progPaneUpdateFlag = 1; - // Invoke the timer only once to exclude overlaps - // The timer will be restarted in 'ProgressPaneUpdateTimerElapsed' - _progPaneUpdateTimer = new Timer( new TimerCallback(ProgressPaneUpdateTimerElapsed), null, UpdateTimerThreshold, Timeout.Infinite); + // The timer will be auto restarted every 'UpdateTimerThreshold' ms + _progPaneUpdateTimer = new Timer( new TimerCallback(ProgressPaneUpdateTimerElapsed), null, UpdateTimerThreshold, UpdateTimerThreshold); } } @@ -107,7 +110,7 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt /// /// - /// TimerCallback for _progPaneUpdateTimer to update 'progPaneUpdateFlag' and restart the timer + /// TimerCallback for '_progPaneUpdateTimer' to update 'progPaneUpdateFlag' /// /// @@ -116,8 +119,6 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt ProgressPaneUpdateTimerElapsed(object sender) { Interlocked.CompareExchange(ref progPaneUpdateFlag, 1, 0); - - _progPaneUpdateTimer?.Change(UpdateTimerThreshold, Timeout.Infinite); } private @@ -214,9 +215,9 @@ class ConsoleHostUserInterface : System.Management.Automation.Host.PSHostUserInt private ProgressPane _progPane = null; private PendingProgress _pendingProgress = null; // The timer set up 'progPaneUpdateFlag' every 'UpdateTimerThreshold' milliseconds to update 'ProgressPane' - private Timer _progPaneUpdateTimer; + private Timer _progPaneUpdateTimer = null; private const int UpdateTimerThreshold = 200; - private int progPaneUpdateFlag; + private int progPaneUpdateFlag = 0; } } // namespace