Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: [SEMI-SOLVED] I Think I Found A Memory Leak  (Read 18649 times)

0 Members and 1 Guest are viewing this topic.

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: I Think I Found A Memory Leak
« Reply #15 on: January 17, 2013, 06:18:21 pm »
Quote
I looked at it and it's an abstract base class with only a one private function, and two protected functions.
I was talking about the stuff hidden inside GlResource.cpp. And "make public" meant "write the functions that you need, using this stuff", not just "change a 'private' to 'public'" ;)

Don't bother investigating in the destructors, there's no mistake in the code. The "leak" happens because each new thread creates an internal/hidden context, but cannot destroy it when the thread ends. So contexts accumulate until all SFML resources have been destroyed (there's a global reference counter which triggers the globalCleanup() function).

Quote
Yes the context management is really black magic . I already tried going through it many times but gave up after a few hours ^^.
I don't like this part of the code, but at least it works ;D

Quote
What? Since when can you not be notified of a thread terminating? There are ways to do this...
How? I have no knowledge about the thread itself, it's created by the user. I only "know" that I'm in a new thread because I use a thread-local variable.
Laurent Gomila - SFML developer

7krs

  • Newbie
  • *
  • Posts: 39
    • View Profile
    • Email
Re: I Think I Found A Memory Leak
« Reply #16 on: January 17, 2013, 07:40:19 pm »
(there's a global reference counter which triggers the globalCleanup() function).

It was found. Also this:

    sf::priv::GlContext* getInternalContext()
    {
        if (!hasInternalContext())
        {
            internalContext = sf::priv::GlContext::create();
            sf::Lock lock(internalContextsMutex);
            internalContexts.insert(internalContext);
        }

        return internalContext;
    }

You seem to delete the entire set when there are 0 contexts. I'm busy creating a function that deletes a single context + remove the pointer from the set with the only parameter being the Texture. Tried several times now, kinda like walking into a maze...

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: I Think I Found A Memory Leak
« Reply #17 on: January 17, 2013, 08:23:16 pm »
I'm really sorry, I don't have time to write and test a solution for you.

I think you should:
- create a public function named "releaseThreadContext"
- inside this function, you remove internalContext  from the internalContexts array and then you delete it

But this will work only if you nothing after executing this function (ie. not calling setActive(false) or destroying a render-window/render-texture).
Laurent Gomila - SFML developer

7krs

  • Newbie
  • *
  • Posts: 39
    • View Profile
    • Email
Re: I Think I Found A Memory Leak
« Reply #18 on: January 17, 2013, 10:37:30 pm »
I'm really sorry, I don't have time to write and test a solution for you.

I think you should:
- create a public function named "releaseThreadContext"
- inside this function, you remove internalContext  from the internalContexts array and then you delete it

But this will work only if you nothing after executing this function (ie. not calling setActive(false) or destroying a render-window/render-texture).

#ifdef LINUX
#include <X11/Xlib.h>
#endif


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

int main()
{
    #ifdef LINUX
    XInitThreads(); // Required on Linux to handle multiple threads with windowing.
    #endif


    // The drawer thread, active during the entire program's lifetime, constant drawing ////////////
    std::thread([&]()
                {
                    sf::RenderWindow wnd(sf::VideoMode(300, 200, 32), "Title");
                    wnd.setFramerateLimit(1);
                    while (true)
                    {
                        wnd.clear();
                        wnd.display();
                    }
                }
                ).detach();
    //////////////////////////////////////////////////////////////////////////////////////////////


    // Simulation of switching states ////////////////////////////////////////////////////////////
    while (true)
    {
        std::thread([&]()
                    {
                        sf::RenderTexture *rt = new sf::RenderTexture;
                        delete rt;
                        sf::GlResource::releaseThreadContext();
                        std::this_thread::sleep_for(std::chrono::seconds(1));
                    } // rt should be destroyed here, but its effect on the memory remains.
                    ).join();
    }
    //////////////////////////////////////////////////////////////////////////////////////////////
    return 0;
}
 


I gave sf::GlResource the following public static member function:
static void releaseThreadContext();
---
void GlResource::releaseThreadContext()
{
        priv::GlContext::releaseThreadContext();
}
 

So I thus gave sf::priv::GlContext the following public static member function:
static void releaseThreadContext();
---
void GlContext::releaseThreadContext()
{
        sf::Lock lock(internalContextsMutex);
        internalContexts.erase(internalContext);
        delete internalContext;
}
 


However, no resources are freed in my testing scenario. I wonder what I am doing horribly wrong.


EDIT:
I replaced the
void GlContext::releaseThreadContext();
internals with the following:

sf::Lock lock(internalContextsMutex);
    for (std::set<GlContext*>::iterator it = internalContexts.begin(); it != internalContexts.end(); ++it)
            delete *it;
    internalContexts.clear();
 
And it seems to work even when I have already-existing sprites being drawn, among other things. I'll now test it in my program to see if it works just as well there.

EDIT2: TOTALLY AWESOME! IT WORKS! Aaaah finally, after all these hours (so happy). I basically call sf::GlResource::releaseThreadContext(); after my drawing thread finishes, so another drawing thread can start drawing. This works perfectly, no segfaults, no error messages from OpenGL, nothing. Absolutely wonderful. Magnificent-> *good feels* :). But now it would need to be tested thoroughly. I really hope that this is something that stays eternally stable. It should also be a nice feature to add officially to make SFML more thread-friendly... Aaah, now I can rest :D
« Last Edit: January 17, 2013, 11:43:00 pm by 7krs »

binary1248

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1405
  • I am awesome.
    • View Profile
    • The server that really shouldn't be running
Re: I Think I Found A Memory Leak
« Reply #19 on: January 18, 2013, 01:44:22 pm »
How? I have no knowledge about the thread itself, it's created by the user. I only "know" that I'm in a new thread because I use a thread-local variable.
There are only 2 ways a thread can terminate, either the thread function ends normally in which case execution will return to ThreadImpl::entryPoint(void* userData), or the user chooses to terminate the thread with terminate() in which case ThreadImpl::terminate() will be called. In both cases SFML is aware that the thread ends, you just don't make note of it and carry on with the application execution.

I know you don't want to make System depend on Window, so in this case you would just have to create a generic API that can notify observers that a thread has ended and it is up to them what they want to do. This would also be useful when attaching dynamic memory to TLS objects since you can be sure that they will be deallocated when the thread ends. Right now if you terminate a thread, it will cause a leak because the deallocator inside the thread function isn't called.

And regarding std::set<sf::priv::GlContext*> internalContexts;:
You already store all contexts inside this mutex-guarded set. You just don't have any association between which contexts belong to which thread and whether or not those threads are still alive. If you stored the association inside a std::map then you could easily get rid of dead contexts even during the runtime of the application. You just have to be notified of a thread terminating as per above and then remove the associated context respectively.

You probably don't want to focus on making the threading API more versatile right now, but advanced thread usage is impossible which such a limited API. I don't have the same problem as 7krs right now because as you know I avoid threading where possible especially for draw related operations. But some day someone will come with a really good use case and providing him with such a weak API will really deter the more "serious" library users.

But now it would need to be tested thoroughly. I really hope that this is something that stays eternally stable. It should also be a nice feature to add officially to make SFML more thread-friendly...
Getting rid of all internal GLContexts every time a thread ends isn't a clean solution. If it weren't for context sharing (which I despise with passion) your OpenGL resources would also disappear every time a thread ends. Not only that, GLContext creation is relatively expensive and doing it more times than needed will significantly impact the performance of your application. If you are content with this solution for your own needs (where I assume memory consumption is more important than application execution speed) then this is an acceptable solution. However most users don't share this requirement and your code probably will not be used in SFML as it is not a clean solution.
SFGUI # SFNUL # GLS # Wyrm <- Why do I waste my time on such a useless project? Because I am awesome (first meaning).

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: I Think I Found A Memory Leak
« Reply #20 on: January 18, 2013, 02:51:56 pm »
Quote
IT WORKS
You delete all the internal contexts, even those which might be in use in other threads.

Quote
There are only 2 ways a thread can terminate, either the thread function ends normally in which case execution will return to ThreadImpl::entryPoint(void* userData), or the user chooses to terminate the thread with terminate() in which case ThreadImpl::terminate() will be called. In both cases SFML is aware that the thread ends, you just don't make note of it and carry on with the application execution.
I was talking about threads in general, not sf::Thread. Look, he's using std::thread in his example.
Laurent Gomila - SFML developer

7krs

  • Newbie
  • *
  • Posts: 39
    • View Profile
    • Email
Re: I Think I Found A Memory Leak
« Reply #21 on: January 18, 2013, 03:10:33 pm »
You delete all the internal contexts, even those which might be in use in other threads.

I realize this, for trying to delete the thread-local context does not seem to work. The weird thing that surprised me is that all the other drawers stayed valid, they drew their rendertextures and updated them succesfully. After this the screen_drawer rendered the sprites of these textures succesfully.

Getting rid of all internal GLContexts every time a thread ends isn't a clean solution. If it weren't for context sharing (which I despise with passion) your OpenGL resources would also disappear every time a thread ends. Not only that, GLContext creation is relatively expensive and doing it more times than needed will significantly impact the performance of your application. If you are content with this solution for your own needs (where I assume memory consumption is more important than application execution speed) then this is an acceptable solution. However most users don't share this requirement and your code probably will not be used in SFML as it is not a clean solution.

I am not constantly switching states if that is what you think, I'm aiming for different states that last a long time, and I do not want to create many shared drawables and a single drawer thread. So the GlContexts are basically created upon entering a new major state in my program. So deleting all contexts is useful in my situation, but I would agree (if you imply); that it is more useful to let every thread kill its own context.

binary1248

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1405
  • I am awesome.
    • View Profile
    • The server that really shouldn't be running
Re: I Think I Found A Memory Leak
« Reply #22 on: January 18, 2013, 03:15:16 pm »
I was talking about threads in general, not sf::Thread. Look, he's using std::thread in his example.
Well in that case I don't think it is even safe, as Microsoft documentation states that you need to create and manage threads using one of their library functions to be able to use TLS without unintended side effects. I don't know how std::thread is implemented but I would not try to and mix and match if I don't need to.

The weird thing that surprised me is that all the other drawers stayed valid, they drew their rendertextures and updated them succesfully. After this the screen_drawer rendered the sprites of these textures succesfully.
This is because Laurent inserted ensureGLContext(); checks everywhere and new GLContexts are created every time they are needed and no valid one exists.
SFGUI # SFNUL # GLS # Wyrm <- Why do I waste my time on such a useless project? Because I am awesome (first meaning).

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: I Think I Found A Memory Leak
« Reply #23 on: January 18, 2013, 03:20:53 pm »
Quote
Well in that case I don't think it is even safe, as Microsoft documentation states that you need to create and manage threads using one of their library functions to be able to use TLS without unintended side effects. I don't know how std::thread is implemented but I would not try to and mix and match if I don't need to.
How could you create a thread on Windows without using one of the functions provided by the Windows API?
Laurent Gomila - SFML developer

binary1248

  • SFML Team
  • Hero Member
  • *****
  • Posts: 1405
  • I am awesome.
    • View Profile
    • The server that really shouldn't be running
Re: I Think I Found A Memory Leak
« Reply #24 on: January 18, 2013, 04:16:39 pm »
What my point was is that you don't have any information about how std::thread is implemented. It might be implemented differently in different runtimes. I know that ultimately they all need to use some kind of kernel functions to create a thread, but if e.g. std::thread was implemented using CreateThread as opposed to _beginthreadex then more care has to be taken that strange things don't start to happen when using SFML constructs inside threads because the assumption was that SFML has its own threading facility that is based around _beginthreadex and thus it is safe to use library functions that are permitted only when using that method of thread creation.
SFGUI # SFNUL # GLS # Wyrm <- Why do I waste my time on such a useless project? Because I am awesome (first meaning).

7krs

  • Newbie
  • *
  • Posts: 39
    • View Profile
    • Email
Re: I Think I Found A Memory Leak
« Reply #25 on: January 18, 2013, 08:45:18 pm »
This is because Laurent inserted ensureGLContext(); checks everywhere and new GLContexts are created every time they are needed and no valid one exists.

I didn't know that function was called upon each draw call, explains a lot, thanks.
I figure that this solves my issue quite well, so I'm done here.

Thanks for the hints Laurent!

7krs

  • Newbie
  • *
  • Posts: 39
    • View Profile
    • Email
Re: [PROBABLY-SOLVED] I Think I Found A Memory Leak
« Reply #26 on: April 22, 2013, 10:23:13 pm »
Decided that a lib should at adapt to me, not the other way around. So I started messing around...
I know this is an old thread, but I had to bump it because I've found what I believe to be a very good fix that may help anyone with the same problem. (We all want improvement right?  :-X)

in sf::priv::GlContext we define the static method "clean":

// Notifies SFML that a thread is ending, and that the context needs to be deleted.
void GlContext::clean()
{
        sf::Lock lock(internalContextsMutex);

    GlContext *ptr = getInternalContext();
    delete ptr;
    internalContexts.erase(ptr);

//      std::cout << "The following contexts are present:\n";
//      for (std::set<GlContext*>::iterator it = internalContexts.begin(); it != internalContexts.end(); ++it)
//      {
//              std::cout << "\t" << *it << std::endl;
//      }
}

The std::couts are for checking if there are garbage contexts left in the internalContexts set.
In GlResource we define a static member function that calls priv::GlContext::clean();.


When sf::GlResource::releaseThreadResource(); is called at the end of a thread, the resources are cleared.
Here's my test code:

#include <SFML/Graphics.hpp>
#include <thread>
#include <cstdio>
#include <X11/Xlib.h>


int main()
{
    XInitThreads();
    sf::RenderWindow wnd(sf::VideoMode(800, 600, 32), "Title", sf::Style::Close);
    while (true)
    {
        std::getchar();

        std::thread([&wnd]()
                    {
                        wnd.setActive(true);
                        wnd.clear(sf::Color::Blue);
                        wnd.display();
                        wnd.setActive(false);

                        // THIS ///////////////////////////////////////////////////////////////
                        sf::GlResource::releaseThreadResource();
                        // //////// ///////////////////////////////////////////////////////////////
                    }).join();

        std::getchar();

        wnd.setActive(true);
        wnd.clear(sf::Color::Green);
        wnd.display();
        wnd.setActive(false);

    }
    return 0;
}
 

SFML snapshot: LaurentGomila-SFML-86897a8, downloaded 22nd of April 2013.
OS: Linux Mint 14 (MATE desktop)
Compiler: GNU GCC
IDE: Code::Blocks
Compiler flags: -std=c++0x -Wextra -Wall -g
Linker: -lpthread /usr/lib/libsfml-audio-d.so /usr/lib/libsfml-graphics-d.so /usr/lib/libsfml-window-d.so /usr/lib/libsfml-system-d.so

Please correct me if/where I am wrong so I can try and fix it.