Fixes #2872 - Driver memory leaks #2885
Conversation
|
@BDisp, I'm trying to figure out the /// <summary>
/// Checks if there are any outstanding timers that need to be processed. Called from the driver's <see cref="IMainLoopDriver"/>.
/// </summary>
/// <param name="wait">If <see langword="false"/> <paramref name="waitTimeout"/> will be set to 0.</param>
/// <param name="waitTimeout">Returns the number of milliseconds remaining in the current timer.</param>
/// <returns></returns>
public bool CheckTimers (bool wait, out int waitTimeout)
{
if (wait == false) {
throw new InvalidOperationException ("CheckTimers should not be called with wait == false");
}
long now = DateTime.UtcNow.Ticks;
if (_timeouts.Count > 0) {
waitTimeout = (int)((_timeouts.Keys [0] - now) / TimeSpan.TicksPerMillisecond);
if (waitTimeout < 0)
// This avoids 'poll' waiting infinitely if 'waitTimeout < 0' until some action is detected
// This can occur after IMainLoopDriver.Wakeup is executed where the pollTimeout is less than 0
// and no event occurred in elapsed time when the 'poll' is start running again.
waitTimeout = 0;
return true;
} else {
waitTimeout = -1;
}
if (!wait) {
waitTimeout = 0;
}
int ic;
lock (_idleHandlers) {
ic = _idleHandlers.Count;
}
return ic > 0;
}I have looked over all the code in the project, including unit tests, and I can't find any cases where it will ever bet set to I bubbles up to:
/// <summary>
/// Run one iteration of the <see cref="MainLoop"/>.
/// </summary>
/// <param name="state">The state returned by <see cref="Begin(Toplevel)"/>.</param>
/// <param name="wait">If <see langword="true"/> will execute the <see cref="MainLoop"/> waiting for events. If <see langword="false"/>
/// the method will return after a single iteration.</param>
/// <param name="firstIteration">Set to <see langword="true"/> if this is the first run loop iteration. Upon return,
/// it will be set to <see langword="false"/> if at least one iteration happened.</param>
public static void RunMainLoopIteration (ref RunState state, bool wait, ref bool firstIteration)
{
I added the So, I believe this whole Am I missing something? |
|
You are right. I also never seen it as false, but it's an option. If false the timeout will be 0 and the loop will continue infinitely. |
|
@BDisp, also nobody ever checks for a waitTimeout == -1. In fact, both Windows and NetDriver have a bug w.r.t this because _keyReady.Wait will be called with a timeout of -1! bool IMainLoopDriver.EventsPending ()
{
_waitForProbe.Set ();
if (_mainLoop.CheckTimersAndIdleHandlers (out var waitTimeout)) {
return true;
}
try {
if (!_tokenSource.IsCancellationRequested) {
// BUGBUG: CheckTimers can return false with waitTimeOut set to -1 if there
// are no idle handlers or timers. The code below doesn't account for this.
_keyReady.Wait (waitTimeout, _tokenSource.Token);
}
} catch (OperationCanceledException) {
return true;
} finally {
_keyReady.Reset ();
}
if (!_tokenSource.IsCancellationRequested) {
return _inputResult.Count > 0 || _mainLoop.CheckTimersAndIdleHandlers (out _);
}
_tokenSource.Dispose ();
_tokenSource = new CancellationTokenSource ();
return true;
}I think I can change CheckTimers to always set waitTimeout = 0 if there are no active timers or idle handlers. Right? |
|
Here's my proposed new function on /// <summary>
/// Called from <see cref="IMainLoopDriver.EventsPending"/> to check if there are any outstanding timers or idle handlers.
/// </summary>
/// <param name="waitTimeout">Returns the number of milliseconds remaining in the current timer (if any).</param>
/// <returns><see langword="true"/> if there is a timer or idle handler active.</returns>
public bool CheckTimersAndIdleHandlers (out int waitTimeout)
{
var now = DateTime.UtcNow.Ticks;
waitTimeout = 0;
lock (_timeouts) {
if (_timeouts.Count > 0) {
waitTimeout = (int)((_timeouts.Keys [0] - now) / TimeSpan.TicksPerMillisecond);
if (waitTimeout < 0) {
// This avoids 'poll' waiting infinitely if 'waitTimeout < 0' until some action is detected
// This can occur after IMainLoopDriver.Wakeup is executed where the pollTimeout is less than 0
// and no event occurred in elapsed time when the 'poll' is start running again.
waitTimeout = 0;
}
return true;
}
}
// There are no timers set, check if there are any idle handlers
lock (_idleHandlers) {
return _idleHandlers.Count > 0;
}
} |
I don't recommend that. Return -1 is normally used to wait until some keystroke occurs. If it's necessary to interrupt, then the |
|
Please explain how it makes sense to pass -1 to _keyReady.Wait? |
That's the way how |
I see now:
Very confusing and a great example of where just a few comments in the code would be super helpful. New version: /// <summary>
/// Called from <see cref="IMainLoopDriver.EventsPending"/> to check if there are any outstanding timers or idle handlers.
/// </summary>
/// <param name="waitTimeout">Returns the number of milliseconds remaining in the current timer (if any). Will be -1 if
/// there are no active timers.</param>
/// <returns><see langword="true"/> if there is a timer or idle handler active.</returns>
public bool CheckTimersAndIdleHandlers (out int waitTimeout)
{
var now = DateTime.UtcNow.Ticks;
waitTimeout = 0;
lock (_timeouts) {
if (_timeouts.Count > 0) {
waitTimeout = (int)((_timeouts.Keys [0] - now) / TimeSpan.TicksPerMillisecond);
if (waitTimeout < 0) {
// This avoids 'poll' waiting infinitely if 'waitTimeout < 0' until some action is detected
// This can occur after IMainLoopDriver.Wakeup is executed where the pollTimeout is less than 0
// and no event occurred in elapsed time when the 'poll' is start running again.
waitTimeout = 0;
}
return true;
}
// ManualResetEventSlim.Wait, which is called by IMainLoopDriver.EventsPending, will wait indefinitely if
// the timeout is -1.
waitTimeout = -1;
}
// There are no timers set, check if there are any idle handlers
lock (_idleHandlers) {
return _idleHandlers.Count > 0;
}
} |
|
As I was fixing mem leak this am in windows driver I found the tearing on resize no longer happens if we use the input event! I think we can remove the CheckWinChanges task, at least for recent windows / wt versions! |
Meh. I couldn't get it to work; the sizes given in the event and during Init are still wrong and it's not until the first time the ChangeWin thread runs that the right size is returned. I did document it all better in This will be moot once WindowsDriver is refactored via #2610. |
|
Ok, WindowsDriver is now fixed in this PR. now on to NetDriver:
|
|
FWIW, Performance Profiler doesn't let you specify cmd line args to the Startup Project. I've enabled a ConfigurationManager setting to force it:
|
|
I fixed the most egregious memory leak in Netdriver here. However, the interplay between the threading Tasks in In addition, there still is a very weird memory leak (or allocation of objects that doesn't make sense):
{
"Application.UseSystemConsole" : true
}
You'll see something like this: Click on the +1612 and see this (sort by "Count Diff"): For some reason ~1000 "Task+DelayPromiseWithCancellation" objects are being created. The number is different each run, but not in an deterministic way. I've also noticed that it has something do with mouse vs. keyboard input. But I've not been able to find a pattern. Nor have I figured out how to fix it (obviously). Maybe you can see what's going on? |
Fixes #2872 - Driver memory leaks
Pull Request checklist:
CTRL-K-Dto automatically reformat your files before committing.dotnet testbefore commit///style comments)