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 for OpenGL context leak #705

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@binary1248
Member

binary1248 commented Sep 24, 2014

Supersedes #640 with a non-intrusive mechanism to automatically free thread-local contexts after thread termination.

@rcurtis

This comment has been minimized.

Show comment
Hide comment
@rcurtis

rcurtis Sep 25, 2014

This is failing to build for me on win7/VS2013

4>........\src\SFML\Window\Win32\GlContextImpl.cpp(75): error C3861: '_endthreadex': identifier not found
4>........\src\SFML\Window\Win32\GlContextImpl.cpp(105): error C3861: '_beginthreadex': identifier not found

rcurtis commented Sep 25, 2014

This is failing to build for me on win7/VS2013

4>........\src\SFML\Window\Win32\GlContextImpl.cpp(75): error C3861: '_endthreadex': identifier not found
4>........\src\SFML\Window\Win32\GlContextImpl.cpp(105): error C3861: '_beginthreadex': identifier not found

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 25, 2014

Member

Should be fixed in the latest commit 😉.

Member

binary1248 commented Sep 25, 2014

Should be fixed in the latest commit 😉.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Sep 26, 2014

Member

Testing this on windows there seems to be something going on (looks like a driver issue). @binary1248 has tested the following code and not had any issues. However, myself, @rcurtis, and @Mischa-Alff all reported memory usage increasing.

@Mischa-Alff and myself even have warning popping out in geDEBugger / CodeXL about wglDeleteContext not properly destroying contexts.

I am looking for anyone to test the following code with the bugfix/context_leak branch and let me know the results as I am trying to rule what the problem is. Also please state what GPU you are testing it on please. If you don't want to bother compiling the branch and the following code I have a test executable here that shows the issue on several systems.

#include <SFML/Window.hpp>
#include <SFML/Graphics.hpp>

#include <thread>

void ThreadEntry()
{
    sf::Texture texture;
    texture.loadFromFile("background.png");
}

int main()
{
    sf::Window window({ 500, 500 }, "Context Leak");

    while (true)
    {
        std::thread thread(ThreadEntry);
        thread.join();
    }
}
Member

zsbzsb commented Sep 26, 2014

Testing this on windows there seems to be something going on (looks like a driver issue). @binary1248 has tested the following code and not had any issues. However, myself, @rcurtis, and @Mischa-Alff all reported memory usage increasing.

@Mischa-Alff and myself even have warning popping out in geDEBugger / CodeXL about wglDeleteContext not properly destroying contexts.

I am looking for anyone to test the following code with the bugfix/context_leak branch and let me know the results as I am trying to rule what the problem is. Also please state what GPU you are testing it on please. If you don't want to bother compiling the branch and the following code I have a test executable here that shows the issue on several systems.

#include <SFML/Window.hpp>
#include <SFML/Graphics.hpp>

#include <thread>

void ThreadEntry()
{
    sf::Texture texture;
    texture.loadFromFile("background.png");
}

int main()
{
    sf::Window window({ 500, 500 }, "Context Leak");

    while (true)
    {
        std::thread thread(ThreadEntry);
        thread.join();
    }
}
@rcurtis

This comment has been minimized.

Show comment
Hide comment
@rcurtis

rcurtis Sep 26, 2014

Test info so far:

Leaks non stop on NVIDIA GeForce 210.

No leaks on a Radeon HD 8280E

rcurtis commented Sep 26, 2014

Test info so far:

Leaks non stop on NVIDIA GeForce 210.

No leaks on a Radeon HD 8280E

@sleeparrow

This comment has been minimized.

Show comment
Hide comment
@sleeparrow

sleeparrow Sep 26, 2014

Leaking for me on Windows 7 with an Intel HD Graphics 2000.

sleeparrow commented Sep 26, 2014

Leaking for me on Windows 7 with an Intel HD Graphics 2000.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 26, 2014

Member

Try this code:

#include <SFML/Window.hpp>
#include <SFML/Graphics.hpp>

void ThreadEntry()
{
    sf::Texture texture;
    texture.create(100, 100);
}

int main()
{
    sf::Window window({ 500, 500 }, "Context Leak");

    while (true)
    {
        sf::Thread thread(ThreadEntry);
        thread.launch();
    }
}

Using sf::Thread instead of std::thread ensures that the underlying threading matches with SFML's TLS and threading backend. Mismatches might cause something to go wrong. Also, mention whether you static or dynamic link. I only tested it when static linking.

Member

binary1248 commented Sep 26, 2014

Try this code:

#include <SFML/Window.hpp>
#include <SFML/Graphics.hpp>

void ThreadEntry()
{
    sf::Texture texture;
    texture.create(100, 100);
}

int main()
{
    sf::Window window({ 500, 500 }, "Context Leak");

    while (true)
    {
        sf::Thread thread(ThreadEntry);
        thread.launch();
    }
}

Using sf::Thread instead of std::thread ensures that the underlying threading matches with SFML's TLS and threading backend. Mismatches might cause something to go wrong. Also, mention whether you static or dynamic link. I only tested it when static linking.

@rcurtis

This comment has been minimized.

Show comment
Hide comment
@rcurtis

rcurtis Sep 26, 2014

Is there a compliment for sf::Thread in the .NET bindings?

rcurtis commented Sep 26, 2014

Is there a compliment for sf::Thread in the .NET bindings?

@rcurtis

This comment has been minimized.

Show comment
Hide comment
@rcurtis

rcurtis Sep 26, 2014

After testing the previous code snippet, it does not leak. But changing the line texture.create(100, 100); to texture.loadFromFile("background.png"); leaks just as bad as using a std::thread.

rcurtis commented Sep 26, 2014

After testing the previous code snippet, it does not leak. But changing the line texture.create(100, 100); to texture.loadFromFile("background.png"); leaks just as bad as using a std::thread.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 1, 2014

Member

Unfortunately, I don't see a way to make SFML's TLS implementation "std::thread proof", at least on Windows systems. Even the all mighty boost.thread library say they can't assure TLS destructors are eventually called if you use something else than boost's threads, which kind of make sense.

Technically, it isn't really Windows' fault we have this problem. This stems from the design of the current context management. TLS isn't meant to be used passively. The user spawning a new thread should be the one controlling the TLS, and thus the one who owns it and makes sure that it is allocated and deallocated properly. Unfortunately SFML doesn't pass this responsibility on to the user in the current implementation in regards to the context.

I'll close this PR and remove the branch from the GitHub repository for now (but still keep it locally for archival purposes), since it only seems to work in very specific cases.

@zsbzsb: If you want, you can feel free to reopen #640, although I don't think it is something that will make its way into core SFML. The context management is bound to be changed sooner or later, and this fix is really only necessary (i.e. in order to not have any severe side effects) in very specific use cases, so it would probably be distributed among those that really need it and know how to use it.

Member

binary1248 commented Oct 1, 2014

Unfortunately, I don't see a way to make SFML's TLS implementation "std::thread proof", at least on Windows systems. Even the all mighty boost.thread library say they can't assure TLS destructors are eventually called if you use something else than boost's threads, which kind of make sense.

Technically, it isn't really Windows' fault we have this problem. This stems from the design of the current context management. TLS isn't meant to be used passively. The user spawning a new thread should be the one controlling the TLS, and thus the one who owns it and makes sure that it is allocated and deallocated properly. Unfortunately SFML doesn't pass this responsibility on to the user in the current implementation in regards to the context.

I'll close this PR and remove the branch from the GitHub repository for now (but still keep it locally for archival purposes), since it only seems to work in very specific cases.

@zsbzsb: If you want, you can feel free to reopen #640, although I don't think it is something that will make its way into core SFML. The context management is bound to be changed sooner or later, and this fix is really only necessary (i.e. in order to not have any severe side effects) in very specific use cases, so it would probably be distributed among those that really need it and know how to use it.

@binary1248 binary1248 closed this Oct 1, 2014

@binary1248 binary1248 added s:rejected and removed s:undecided labels Oct 1, 2014

@binary1248 binary1248 deleted the bugfix/context_leak branch Oct 1, 2014

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Oct 2, 2014

Member

@binary1248 I understand, after going through the options the only way to truly fix this is manually context management or my other band-aid fix to to the limitations of deleting contexts from other threads. (the leak on windows is due to the fact you can not delete active contexts from another thread and you can't deactivate a context without creating another which leads to the same problem...)

Member

zsbzsb commented Oct 2, 2014

@binary1248 I understand, after going through the options the only way to truly fix this is manually context management or my other band-aid fix to to the limitations of deleting contexts from other threads. (the leak on windows is due to the fact you can not delete active contexts from another thread and you can't deactivate a context without creating another which leads to the same problem...)

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