-
Notifications
You must be signed in to change notification settings - Fork 681
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
NetDriver is consuming too much CPU resources in ReadConsoleKeyInfo. #3520
Comments
…eadConsoleKeyInfo.
Yeah. We should have zero instances of Task.Delay anywhere in the code. |
Yes it suits. The only one I can't avoid is the |
We can probably use (and probably should use) something like (Auto|Manual)ResetEvent or another synchronization primitive for that, rather than sleeps in method calls that may or may not execute on another thread or even asynchronously at all (you can't control it when using the TAP like this). |
There is already ManualResetEvent for those we can control using like a switch. For the console resizing we have to monitoring periodically. |
…ces in ReadConsoleKeyInfo." This reverts commit 79192ed.
…eadConsoleKeyInfo and in CheckWindowSizeChange.
A timer is much more appropriate than sleeps, if periodic polling is the only reliable means of doing it. |
I opted for a simpler solution taking advantage of the cancellation token by just adding the |
It's not really simpler, though.
Instead, simply have the application own a System.Timers.Timer object, which sits there free-running.
|
Just to show how significant seemingly small details like this really can be, here's a comparison of how much more actual code is created by the Take the following two simple applications that do essentially the same thing: // This is in both applications and is simply what keeps it from exiting immediately.
private static readonly ManualResetEventSlim _appKeepalive = new();
public static async Task Main()
{
while(!_appKeepalive.IsSet)
{
StuffInsideLoop();
await Task.Delay(250).ConfigureAwait(true);
}
}
private static void StuffInsideLoop()
{
ConsoleKeyInfo key = Console.ReadKey(true);
Console.Out.Write(key.KeyChar);
if (key is { Key: ConsoleKey.Q })
{
_appKeepalive.Set();
}
} And using System.Timers.Timer: public static class Program
{
private static readonly System.Timers.Timer _eventDispatchTimer = new(250){ AutoReset = true};
// This is in both applications and is simply what keeps it from exiting immediately.
private static readonly ManualResetEventSlim _appKeepalive = new();
public static void Main()
{
_eventDispatchTimer.Elapsed+= StuffInsideLoop;
_eventDispatchTimer.Start();
_appKeepalive.Wait( -1 );
}
private static void StuffInsideLoop(object? sender, ElapsedEventArgs e)
{
ConsoleKeyInfo key = Console.ReadKey(true);
Console.Out.Write(key.KeyChar);
if (key is { Key: ConsoleKey.Q })
{
_appKeepalive.Set();
}
}
} The first results in 13.5kB of CIL. The second results in 12.5kB of CIL. There's a lot of extra stuff going on for just that little tiny program. And when I compile down to native, the first one is 8kB larger than the second, when optimized and without debug symbols included. That's a lot of executable code, and a lot of run-time resources as well. These effects will be even more significant in an application doing more than just this, like anything referencing Terminal.Gui. |
But with |
You don't even need it with a timer. You simply call Stop or Dispose it or, if the application is exiting, let it die on its own with the AppDomain. But you can pass a CT to it if you want, anyway. But it'd be more advantageous to register for the cancellation callback from the CTS, rather than making a new CT each call, which is what happens. And, wanna know how CancellationTokenSource is implemented? A Timer and a ManualResetEvent, plus a couple other WaitHandles for synchronizing things like the registration callback list (which is an event, but with specific synchronization behavior. But CT is a readonly struct. Every time you pass it, you're making at least one copy. Besides, that's very improper use of the CT and of the Task itself, anyway. You do not return from a canceled task. You throw OperationCanceledException. Returning can result in resource leaks and synchronization issues that can't be traced because you've thrown the handle away without letting the caller know what happene. At that point, a Task is completely pointless and you may as well just use ThreadPool.QueueUserWorkItem instead (don't do that). |
It's not making a new CT anymore. In this commit e03b20e I made it as readonly, like I did in the v1 on #3519. |
That's fine with me. When we get closer to or maybe actually in beta, I'll be happy to take a performance pass over things like this. |
This was already fixed on #3515. |
My laptop looks like a fryer running
NetDriver
version 2, because it is usingTask.Delay
, which should only be used for very short-term cases and not for the entire execution of an application. WhenTask.Delay
is used during the entire application the async must be used to be really awaiting, otherwise it will return immediately without waiting for the elapsed time.The text was updated successfully, but these errors were encountered: