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

Fix memory leaks post game exit #1472

Merged
merged 26 commits into from Mar 22, 2018

Conversation

3 participants
@peppy
Member

peppy commented Mar 19, 2018

This is mostly to reduce nUnit memory/thread usage overhead.

In normal game execution everything is freed when the application exits, so this should not present any discernible improvement to an end user.

@peppy peppy added this to the March 2018 milestone Mar 19, 2018

@@ -33,7 +33,9 @@ namespace osu.Framework.Platform
{
public abstract class GameHost : IIpcHost, IDisposable
{
public GameWindow Window;
public GameWindow Window { get; protected set; }

This comment has been minimized.

@smoogipoo

smoogipoo Mar 19, 2018

Contributor

Shouldn't this be private set;?

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

no, check usage

This comment has been minimized.

@smoogipoo

smoogipoo Mar 19, 2018

Contributor

Interesting... Alright let's go with this for now then.

@@ -614,6 +630,8 @@ protected virtual void Dispose(bool disposing)
public void Dispose()
{
Root = null;

This comment has been minimized.

@smoogipoo

smoogipoo Mar 19, 2018

Contributor

Why is this here and not in Dispose(disposing)?

}
protected override void Dispose(bool isDisposing)
{
Window.WindowStateChanged -= onWindowOnWindowStateChanged;

This comment has been minimized.

@smoogipoo

smoogipoo Mar 19, 2018

Contributor

All the changes in this file shouldn't be necessary anymore right? Since we're disposing the window and the toolkit and shouldn't have a reference held.

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

it's still best practice to do this. there could be unexpected behaviour if, for instance, the window state changes post disposal of the WindowsGameHost (it could fire something bound here that is no longer expected to be in context).

Root?.Dispose();
Activated = null;
Deactivated = null;

This comment has been minimized.

@smoogipoo

smoogipoo Mar 19, 2018

Contributor

These likewise shouldn't be necessary anymore right?

@peppy peppy force-pushed the peppy:memory-fixes branch from ee52f03 to 76c0a36 Mar 19, 2018

@peppy peppy requested a review from Tom94 Mar 19, 2018

@@ -14,7 +14,7 @@ namespace osu.Framework.Threading
/// <summary>
/// Marshals delegates to run from the Scheduler's base thread in a threadsafe manner
/// </summary>
public class Scheduler : IDisposable

This comment has been minimized.

@Tom94

Tom94 Mar 19, 2018

Collaborator

Not sure I like this change. Consider the case, where a ThreadedScheduler is stores as a Scheduler member due to polymorphism. Now, there is no way to explicitly dispose the ThreadedScheduler despite that being perhaps desired.

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

threaded scheduler is used in only one place. if you disagree here i'd rather remove the threaded behaviour from it completely.

This comment has been minimized.

@Tom94

Tom94 Mar 19, 2018

Collaborator

What's so undesirable about keeping Scheduler as IDisposable?

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

feels weird having something disposable when all but one very private use-case do not need disposal. it forces us to call .Dispose() in usages where it's completely unnecessary.

This comment has been minimized.

@Tom94

Tom94 Mar 19, 2018

Collaborator

I don't think it does. Disposal is always done by the finalizer anyway, so calling it explicitly is entirely optional and related to deterministic resource de-allocation.

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

and as such, there's already a finaliser on ThreadedScheduler.

i'd argue if you're constructing a ThreadedScheduler, as the consumer you should be aware of its need for disposal.

This comment has been minimized.

@Tom94

Tom94 Mar 19, 2018

Collaborator

As a consumer you often expect the generic Scheduler class, not some specialized more specific class (and the implementation details coming with it). Allowing explicit disposal strictly adds optional functionality, so I see no reason against having it.

This is like having an abstract AudioTrack class where some derivatives (e.g. BassAudioTrack) require disposal. It'd be crazy to not expose the disposal within AudioTrack, because every consumer will be implemented against the generic AudioTrack rather than the specific BassAudioTrack.

This comment has been minimized.

@peppy

peppy Mar 19, 2018

Member

In this case, I'm saying I'd rather remove ThreadedScheduler (there are better options for threading tasks already) and just make Scheduler have a more limited scope.

ThreadedScheduler is only used by Logger at the moment (and internally uses Threads directly instead of Tasks, which we're moving away from).

This comment has been minimized.

@Tom94

Tom94 Mar 19, 2018

Collaborator

Sounds like a fair resolution to me.

peppy added some commits Mar 19, 2018

resolved

{
try
{
while (!cancellationToken.IsCancellationRequested)

This comment has been minimized.

@smoogipoo

smoogipoo Mar 20, 2018

Contributor

This is going to consume one full thread since there no longer is a Thread.Sleep() in here (as ThreadedScheduler had).

This comment has been minimized.

@peppy

peppy Mar 20, 2018

Member

good catch

if (writerTask != null)
{
cancellationToken.Cancel();
writerTask.Wait(500);

This comment has been minimized.

@smoogipoo

smoogipoo Mar 20, 2018

Contributor

Is there a reason why this is waiting for 500ms, vs just Wait()? I'd expect Flush() to completely flush the log, not potentially only a part of the log.

This comment has been minimized.

@smoogipoo

smoogipoo Mar 20, 2018

Contributor

We should also be disposing the cancellationToken here (it does not implement a finaliser).

This comment has been minimized.

@peppy

peppy Mar 20, 2018

Member

500 was a sane value which allows for writing at least a few megabytes of data. i prefer not having infinite timeouts where possible.

This comment has been minimized.

@peppy

peppy Mar 20, 2018

Member

An example case where this may be a good idea (which is why i added this in the first place) would be if there's a second thread logging new entries faster than the writing can complete and it becomes an endless loop.

}
catch (TaskCanceledException)
{
}

This comment has been minimized.

@smoogipoo

smoogipoo Mar 20, 2018

Contributor

This catch should never occur, since nothing in the try { } will throw this exception.

This comment has been minimized.

@smoogipoo

smoogipoo Mar 20, 2018

Contributor

This catch should be around the Wait(), since that is the method that will give you a task cancelled exception, in the form of an AggregateException. This catch being here is potentially problematic since it is passed to the task constructor.

/// <summary>
/// Pause execution until all logger writes have completed and file handles have been closed.
/// This will also unbind all handlers bound to <see cref="NewEntry"/>.
/// </summary>
public static void Flush()

This comment has been minimized.

@smoogipoo

smoogipoo Mar 20, 2018

Contributor

Is there a reason why this method is coded the way it is? Creating a new task and all that? Can we have another method like Close() or Finalise() or Stop() as part of Logger that stops the task completely, and have one task running for the entire thing?

This comment has been minimized.

@peppy

peppy Mar 20, 2018

Member

Are you saying there should be separate start and stop methods?

This comment has been minimized.

@smoogipoo

smoogipoo Mar 20, 2018

Contributor

The following is the idea I'm going for:

static Logger()
{
    task = new Task(() =>
    {
        while (!stopToken.IsCancellationRequested)
        {
            if (flushToken.IsCancellationRequested)
                while (performUpdate()) { }
            else
                performUpdate();
            Thread.Sleep(50);
        }
    });
};

public Logger.Flush()
{
    flushToken.Cancel();
    while (scheduler.HasItems) { }
}

internal Logger.Stop()
{
    stopToken.Cancel();
    task.Wait();
}

GameHost.Dispose() => Logger.Flush(); Logger.Stop();

Maybe not 100% this since the flush stuff has its own implementation issues, and perhaps it might be better to use a second task in there, but I think the code would be much cleaner this way, and we wouldn't have random tasks/cancellation tokens being constructed on dispose.

This comment has been minimized.

@peppy

peppy Mar 20, 2018

Member

Flush would still have to restart the task. Flush is supposed to keep the logger in an operational state should further logging need to happen (it just ensures we are flushed to a certain point).

This comment has been minimized.

@smoogipoo

smoogipoo Mar 20, 2018

Contributor

Flush wouldn't stop the task in the first place.

peppy added some commits Mar 20, 2018

while (true)
{
if ((Storage != null ? scheduler.Update() : 0) == 0)
writer_idle.Set();

This comment has been minimized.

@smoogipoo

smoogipoo Mar 22, 2018

Contributor

We need to reset writer_idle here:

else
    writer_idle.Reset();
@smoogipoo

This comment has been minimized.

Contributor

smoogipoo commented Mar 22, 2018

👍 Lets do this 🤞

@smoogipoo smoogipoo merged commit d8d4f55 into ppy:master Mar 22, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@peppy peppy deleted the peppy:memory-fixes branch Apr 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment