Fix performance issue with ReadKey changes #621

SeeminglyScience · 2018-02-05T00:45:34Z

Separated the ReadKey changes to be platform specific. This way we don't need a busy loop to wait until a key is available on Windows.
Changed the Unix implementation of ReadKey to wait for a longer period of time between Console.KeyAvailable checks if the user has not pressed a key within the last five seconds. This change was made to ensure the CPU is able to enter low power mode.

@tylerl0706 if you could do the honors of Mac testing again, that'd be awesome.

- Separated the ReadKey changes to be platform specific. This way we don't need a busy loop to wait until a key is available on Windows. - Changed the Unix implementation of ReadKey to wait for a longer period of time between Console.KeyAvailable checks if the user has not pressed a key within the last five seconds. This change was made to ensure the CPU is able to enter low power mode.

SeeminglyScience · 2018-02-05T12:27:21Z

@tylerl0706 mind restarting AppVeyor when you get a minute? Looks like it hung completely this time.

TylerLeonhardt

I'll approve after I try it out on my mac.

TylerLeonhardt · 2018-02-05T17:05:01Z

src/PowerShellEditorServices/Console/ConsoleReadLine.cs


        private PowerShellContext powerShellContext;

        #endregion

        #region Constructors
+        static ConsoleReadLine()
+        {
+            // Maybe we should just include the RuntimeInformation package for FullCLR?


I don't know the answer to this question but I think we should answer it in this PR.

I'm split on it. The compiler directives feel a little messy, especially if we end up using them more. On the other hand including an assembly that's probably going to be included in a whole bunch of modules doesn't feel great either.

I opted to include System.Runtime.InteropServices.RuntimeInformation.dll in PSReadLine so I could build and ship a single assembly that works on all supported platforms - it definitely kept builds/packing simpler.

@lzybkr Yeah that would be ideal for sure. Do you know of a way to do that and remain buildable on Unix? I came up empty last time I looked into it. The blocker was targeting only net46x required Windows (or Mono?) and targeting only netstandard required shims to load properly in Windows PowerShell.

@SeeminglyScience - I haven't done this, but I think you would need to build targets, one for net46x and one for netstandard. I don't run PSReadLine tests on Linux/Mac (and there is very little reason to, the platform specifics are mostly mocked anyway), so haven't dealt with this situation yet.

TylerLeonhardt · 2018-02-05T17:06:14Z

src/PowerShellEditorServices/Console/UnixConsoleOperations.cs

+
+namespace Microsoft.PowerShell.EditorServices.Console
+{
+    internal class UnixConsoleOperations : IConsoleOperations


Does this apply to Unix and Linux?

Yeah, I'm using "Unix" to refer to Mac and Linux. Not sure if there's a better term, but that's what corefx does.

TylerLeonhardt · 2018-02-05T17:33:45Z

src/PowerShellEditorServices/Console/UnixConsoleOperations.cs

+
+            // I tried to replace this library with a call to `stty -echo`, but unfortunately
+            // the library also sets up allowing backspace to trigger `Console.KeyAvailable`.
+            InputEcho.Disable();


This is your library, right?

TylerLeonhardt

Worked on Mac :) signing off on that reason. Others should review as well.

TylerLeonhardt · 2018-02-05T23:35:29Z

The steps I took:

Start debugging a file with break points, make sure you can
- Press F5 to continue to the next breakpoint
- Press F11 to step to the next sequence point
- Never have to hit enter in the integrated console to do any of that
Spam input to see if any input is lost or just feels bad (timeout between checking for a available key may be need to be adjusted)
Run $Host.UI.ReadLineAsSecureString() and ensure plain text characters aren't echoed to the screen

rkeithhill · 2018-02-08T18:36:07Z

src/PowerShellEditorServices/Console/IConsoleOperations.cs

+        /// </returns>
+        Task<ConsoleKeyInfo> ReadKeyAsync(CancellationToken cancellationToken);
+    }
+}


For POSIX compliance, don't we want a newline at the end of the file? See https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

POSIX compliance is sometimes overrated :)

I was just thinking if someone wanted to work on this repo on Linux/macOS. OTOH they'd probably be using PSCore utilities on files and not the native utilities.

@rkeithhill good catch, I usually have the VSCode workspace setting that adds it automatically. Any objections to me adding it here?

Not from me. In fact, add that setting to the .vscode\settings.json file. Or maybe we adopt .editorconfig so that VS 2017 will honor this setting. There's a plugin for VSCode for EditorConfig so that VSCode will also honor the setting.

@rkeithhill added to the settings.json for now, but I agree with translating that to an editor config. I'll make a separate issue for that.

rkeithhill · 2018-02-08T18:45:57Z

src/PowerShellEditorServices/Console/WindowsConsoleOperations.cs

+            try
+            {
+                return
+                    _bufferedKey.HasValue


Can this ever be anything but false? _bufferedKey is private and I'm not seeing how it would get set before we reach this point.

@rkeithhill it's set on line 22. I was trying to avoid the whole temp variable dance from the original implementation. Might have went a little too complicated with it though. @daviwil thoughts?

rkeithhill · 2018-02-08T18:46:16Z

src/PowerShellEditorServices/Console/UnixConsoleOperations.cs

+
+        private async Task<bool> ShortWaitForKey(CancellationToken cancellationToken)
+        {
+            if (await SpinUntilKeyAvailable(SHORT_READ_TIMEOUT))


Any chance we could we get @lzybkr to review this bit? He's more of a perf wonk. :-)

lzybkr · 2018-02-08T19:06:49Z

src/PowerShellEditorServices/Console/UnixConsoleOperations.cs

+        {
+            return await Task<bool>.Factory.StartNew(
+                () => SpinWait.SpinUntil(
+                    () => System.Console.KeyAvailable,


I have a feeling you'll want a short sleep in here, keyboard latency is at least 15ms (see https://danluu.com/keyboard-latency/) so you can consume a bit less CPU by yielding frequently during those 5 seconds.

Added a 30ms sleep between KeyAvailable checks

lzybkr · 2018-02-08T19:21:42Z

src/PowerShellEditorServices/Console/ConsoleReadLine.cs


        private PowerShellContext powerShellContext;

        #endregion

        #region Constructors
+        static ConsoleReadLine()
+        {
+            // Maybe we should just include the RuntimeInformation package for FullCLR?


I opted to include System.Runtime.InteropServices.RuntimeInformation.dll in PSReadLine so I could build and ship a single assembly that works on all supported platforms - it definitely kept builds/packing simpler.

lzybkr · 2018-02-08T19:26:06Z

src/PowerShellEditorServices/Console/IConsoleOperations.cs

+        /// </returns>
+        Task<ConsoleKeyInfo> ReadKeyAsync(CancellationToken cancellationToken);
+    }
+}


POSIX compliance is sometimes overrated :)

lzybkr · 2018-02-08T19:30:18Z

src/PowerShellEditorServices/Console/UnixConsoleOperations.cs

+{
+    internal class UnixConsoleOperations : IConsoleOperations
+    {
+        private const int LONG_READ_DELAY = 400;


Does this feel laggy? 300 is probably fine and would help feel less laggy.

I started with 500 which did feel laggy. 400 didn't feel laggy, but that was through VNC so not super accurate. Either way, I'm definitely more comfortable with 300, I'll get that in. Thanks for your help on this! 😄

Switched to 300ms. The first delay can still feel a little laggy if you type particularly fast and catch it right at the start of the wait, but I don't think it'll be all that noticeable if you aren't looking for it. At least good enough until the issue is fixed in corefx.

- Added final new lines to new files - Added VSCode workspace setting to automatically add final new lines - Changed unix readkey long delay to 300ms from 400ms - Added a short wait timer to unix readkey short delay to reduce CPU consumption

daviwil

Looks awesome! One area of concern though

daviwil · 2018-02-11T17:13:18Z

src/PowerShellEditorServices/Console/WindowsConsoleOperations.cs

+                    _bufferedKey.HasValue
+                        ? _bufferedKey.Value
+                        : await Task.Factory.StartNew(
+                            () => (_bufferedKey = System.Console.ReadKey(intercept: true)).Value);


I think this implementation might have a different effect than the original, but correct me if I'm wrong. The original implementation only stores the buffered key if the cancellation token is cancelled so that it can be returned the next time the ReadKeyAsync method is called.

In the new implementation, we're buffering the key on each read, potentially causing the same key to be returned twice since it gets returned the first time and then buffered to be returned once again?

Patrick just explained how this works to me over Slack, the finally block takes care of clearing the bufferedKey if the CancellationToken hasn't been cancelled :) Looks good!

daviwil · 2018-02-11T17:18:22Z

src/PowerShellEditorServices/Console/WindowsConsoleOperations.cs

+                    _bufferedKey.HasValue
+                        ? _bufferedKey.Value
+                        : await Task.Factory.StartNew(
+                            () => (_bufferedKey = System.Console.ReadKey(intercept: true)).Value);


Patrick just explained how this works to me over Slack, the finally block takes care of clearing the bufferedKey if the CancellationToken hasn't been cancelled :) Looks good!

TylerLeonhardt · 2018-02-20T17:15:28Z

So excited to merge this :) thanks Patrick!

SeeminglyScience requested review from daviwil and TylerLeonhardt February 5, 2018 00:45

TylerLeonhardt reviewed Feb 5, 2018

View reviewed changes

TylerLeonhardt approved these changes Feb 5, 2018

View reviewed changes

TylerLeonhardt requested a review from rkeithhill February 8, 2018 17:55

rkeithhill reviewed Feb 8, 2018

View reviewed changes

lzybkr reviewed Feb 8, 2018

View reviewed changes

Addressing feedback

33a5514

- Added final new lines to new files - Added VSCode workspace setting to automatically add final new lines - Changed unix readkey long delay to 300ms from 400ms - Added a short wait timer to unix readkey short delay to reduce CPU consumption

daviwil reviewed Feb 11, 2018

View reviewed changes

daviwil approved these changes Feb 11, 2018

View reviewed changes

rkeithhill approved these changes Feb 11, 2018

View reviewed changes

TylerLeonhardt merged commit 406f10b into PowerShell:master Feb 20, 2018

Fix performance issue with ReadKey changes #621

Fix performance issue with ReadKey changes #621

Uh oh!

Conversation

SeeminglyScience commented Feb 5, 2018

Uh oh!

SeeminglyScience commented Feb 5, 2018

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviwil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Feb 20, 2018

Uh oh!

Uh oh!

TylerLeonhardt Feb 5, 2018 •

edited

Loading