veandco / go-sdl2

SDL2 binding for Go
https://godoc.org/github.com/veandco/go-sdl2
BSD 3-Clause "New" or "Revised" License
2.2k stars 218 forks source link

Memory Leak with PollEvent #398

Closed krazzei closed 5 years ago

krazzei commented 5 years ago

Hey I seem to be running into a memory leak issue with PollEvent. Here is a simple example:

for running {
    for event = sdl.PollEvent(); event != nil; event = sdl.PollEvent() {
        switch event.(type) {
        case *sdl.QuitEvent:
            running = false
        }
    }

    window.GLSwap()
}

if you set GLSetSwapInterval(0) this problem is more apparent. You can see memory keeps increasing in task manager or activity monitor (I haven't tested on linux). If you have pprof write heap profile to a file you can also see it allocating memory:

File: pollEvents.exe
Type: inuse_space
Time: Apr 2, 2019 at 6:56pm (CDT)
Entering interactive mode (type "help" for commands, "o" for options)
(pprof) top
Showing nodes accounting for 3.50MB, 100% of 3.50MB total
      flat  flat%   sum%        cum   cum%
    3.50MB   100%   100%     3.50MB   100%  github.com/veandco/go-sdl2/sdl.PollEvent.func1
         0     0%   100%     3.50MB   100%  github.com/veandco/go-sdl2/sdl.PollEvent
         0     0%   100%     3.50MB   100%  main.main
         0     0%   100%     3.50MB   100%  runtime.main

And if we list that function in pprof:

ROUTINE ======================== github.com/veandco/go-sdl2/sdl.PollEvent.func1 in D:\Code\go\pkg\mod\github.com\veandco\go-sdl2@v0.3.0\sdl\events.go
    3.50MB     3.50MB (flat, cum)   100% of Total
         .          .    866:}
         .          .    867:
         .          .    868:// PollEvent polls for currently pending events.
         .          .    869:// (https://wiki.libsdl.org/SDL_PollEvent)
         .          .    870:func PollEvent() Event {
    3.50MB     3.50MB    871:   ret := C.SDL_PollEvent(&cevent)
         .          .    872:   if ret == 0 {
         .          .    873:           return nil
         .          .    874:   }
         .          .    875:   return goEvent((*CEvent)(unsafe.Pointer(&cevent)))
         .          .    876:}

I'm not sure what is causing this, I tried to move ret out of the function like what was done with cevent but that didn't seem to do anything. I tried to use go tool compile -m on the event.go file in go-sdl to see if anything escapes to heap but that is tricky due to the use of C.

System Info: SDL 2.0.9 Go 1.12 Windows 10 Version 1809 (gcc 8.3.0 built by MSYS2) AND OS 10.14.3 (same Go version, same SDL version) (clang 10.0.1) Not going to lie, I'm using goland so I'm not sure what my build command is. I also haven't tested this in C at all, I would be shocked if there is a leak in SDL itself though (but could definitely be a thing) Here is my full example program:

package main

import (
    "fmt"
    "github.com/veandco/go-sdl2/sdl"
    "log"
    "os"
    "runtime/pprof"
)

func main() {
    var window *sdl.Window
    var context sdl.GLContext
    var event sdl.Event
    var running = true
    var err error

    if err := sdl.Init(sdl.INIT_EVERYTHING); err != nil {
        panic(err)
    }

    window, err = sdl.CreateWindow("test", sdl.WINDOWPOS_CENTERED, sdl.WINDOWPOS_CENTERED, 1280, 720, sdl.WINDOW_OPENGL)
    if err != nil {
        panic(err)
    }

    context, err = window.GLCreateContext()
    if err != nil {
        panic(err)
    }
    if context == nil {
        fmt.Println(sdl.GetError())
    }

    // Having an unlimited frame rate helps illustrate the problem.
    err = sdl.GLSetSwapInterval(0)
    if err != nil {
        panic(err)
    }
    defer window.Destroy()
    defer sdl.GLDeleteContext(context)
    defer sdl.Quit()

    for running {
        for event = sdl.PollEvent(); event != nil; event = sdl.PollEvent() {
            switch event.(type) {
            case *sdl.QuitEvent:
                running = false
            }
        }

        window.GLSwap()
    }

    f, err := os.Create("test.prof")
    if err != nil {
        log.Fatal(err)
    }
    pprof.WriteHeapProfile(f)
    f.Close()
}
veeableful commented 5 years ago

Hi @krazzei, I couldn't reproduce the issue with the code on my Ubuntu 19.04 beta. I'm currently abroad and that's the only OS I have right now but I'll try again on my PC and/or Macs once I get back in two days :smiley:

veeableful commented 5 years ago

I tried again on a fresh Fedora 30 Beta distro and it does seem to leak, even when I call SDL2's functions directly like this:

package main

// #cgo windows LDFLAGS: -lSDL2
// #cgo linux freebsd darwin pkg-config: sdl2
// #include <SDL.h>
import "C"

func main() {
        C.SDL_Init(C.SDL_INIT_EVERYTHING)

        window := C.SDL_CreateWindow(C.CString("test"), C.SDL_WINDOWPOS_CENTERED, C.SDL_WINDOWPOS_CENTERED, 1280, 720, C.SDL_WINDOW_OPENGL)
        context := C.SDL_GL_CreateContext(window)
        C.SDL_GL_SetSwapInterval(0)

        for {
                var cevent C.SDL_Event
                C.SDL_PollEvent(&cevent)
                C.SDL_GL_SwapWindow(window, 0);
        }

        C.SDL_GL_DeleteContext(context)
        C.SDL_DestroyWindow(window)
        C.SDL_Quit()
}

and when I write the C version, there doesn't seem to be a problem:

#include <SDL2/SDL.h>

int main()
{
    SDL_Window *window;
    SDL_GLContext context;
    int running = 1;

    SDL_Init(SDL_INIT_EVERYTHING);

    window = SDL_CreateWindow("test", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 1280, 720, SDL_WINDOW_OPENGL);

    context = SDL_GL_CreateContext(window);

    SDL_GL_SetSwapInterval(0);

    while (running) {
        SDL_Event event;

        while (SDL_PollEvent(&event)) {
            switch (event.type) {
                case SDL_QUIT:
                    running = 0;
                    break;
            }
        }

        SDL_GL_SwapWindow(window);
    }

    SDL_GL_DeleteContext(context);
    SDL_DestroyWindow(window);
    SDL_Quit();
}

I'm not sure what is causing this :thinking:. I might need to ask around.

krazzei commented 5 years ago

That's super odd. I wonder if Go doesn't know what to do with the C.int returned from PollEvent so it always stores it on the heap? Good to know that the C version doesn't have this problem, I assume you are referencing the same SDL libraries between Go and C?

I also wonder why it shows on Fedora but not Ubuntu. Maybe SDL was built differently?

veeableful commented 5 years ago

Yup, I used the same SDL2 libraries. Maybe I did it wrong somehow on Ubuntu 19.04 beta. I'll try again later. I also suspect it to be a cgo-related problem, perhaps related to threads.

It seems the leak happens if I call SDL_GL_SwapWindow. If I uncomment it, it doesn't happen:

package main

// #cgo windows LDFLAGS: -lSDL2
// #cgo linux freebsd darwin pkg-config: sdl2
// #include <SDL.h>
import "C"
import "runtime"

func main() {
    runtime.LockOSThread() // need to make sure everything runs on main thread for SDL_PollEvent

        C.SDL_Init(C.SDL_INIT_EVERYTHING)

        window := C.SDL_CreateWindow(C.CString("test"), C.SDL_WINDOWPOS_CENTERED, C.SDL_WINDOWPOS_CENTERED, 1280, 720, C.SDL_WINDOW_OPENGL)
        context := C.SDL_GL_CreateContext(window)
        C.SDL_GL_SetSwapInterval(0)

        for {
                var cevent C.SDL_Event
                C.SDL_PollEvent(&cevent)
                //C.SDL_GL_SwapWindow(window, 0); // uncomment this causes memory leak
        }

        C.SDL_GL_DeleteContext(context)
        C.SDL_DestroyWindow(window)
        C.SDL_Quit()
}

This one is on macOS (PS. Sorry I kept changing OS, I seem to always be at different places with different hardware available when I'm checking on this issue 😅)

veeableful commented 5 years ago

I found out that re-unlocking OS thread and setting GOMAXPROCS to 1 prevents the memory leak but I don't know why...

package main

import (
    "fmt"
    "log"
    "os"
    "runtime"
    "runtime/pprof"

    "github.com/veandco/go-sdl2/sdl"
)

func main() {
    var window *sdl.Window
    var context sdl.GLContext
    var running = true
    var err error

    runtime.UnlockOSThread()
    runtime.GOMAXPROCS(1)

    if err := sdl.Init(sdl.INIT_EVERYTHING); err != nil {
        panic(err)
    }

    window, err = sdl.CreateWindow("test", sdl.WINDOWPOS_CENTERED, sdl.WINDOWPOS_CENTERED, 1280, 720, sdl.WINDOW_OPENGL)
    if err != nil {
        panic(err)
    }

    context, err = window.GLCreateContext()
    if err != nil {
        panic(err)
    }
    if context == nil {
        fmt.Println(sdl.GetError())
    }

    // Having an unlimited frame rate helps illustrate the problem.
    err = sdl.GLSetSwapInterval(0)
    if err != nil {
        panic(err)
    }
    defer window.Destroy()
    defer sdl.GLDeleteContext(context)
    defer sdl.Quit()

    for running {
        for event := sdl.PollEvent(); event != nil; event = sdl.PollEvent() {
            switch event.(type) {
            case *sdl.QuitEvent:
                running = false
            }
        }

        window.GLSwap()
    }

    f, err := os.Create("test.prof")
    if err != nil {
        log.Fatal(err)
    }
    pprof.WriteHeapProfile(f)
    f.Close()
}
krazzei commented 5 years ago

I wonder if it makes another thread and copies the event queue but doesn't clean up the previous thread's event queue?

Thanks for looking into this!

flga commented 5 years ago

Did a quick test and although I do see allocations (as expected) I don't see a leak. Try adding a call to runtime.GC() after the swap and check if you still see it.

I'm running v0.3.1-0.20190402152124-61ef6c98c6b1

(I also removed the heap profile)

flga commented 5 years ago

I'm nowhere near qualified to give a definitive answer, but i'll share my point of view. The allocations you are seeing are normal, it's the pointers to cevent passed to cgo.

Since we're in a hot loop calling swap, go is too busy to run gc, and defers it until it is absolutely necessary. It is not a leak, it's just a case of lazy gc.

This is further corroborated by the fact that manual GC triggering keeps a stable memory footprint, and also from the fact that even without calling it manually, it eventually stabilizes.

Go will let the heap grow, as long as it doesn't get out of control (which by default is 2x the previous size after gc, iirc).

veeableful commented 5 years ago

I'm not familiar with how Go works internally but you are probably right @flga. I have discovered that if I run the program without the tweaks, the memory starts lower around 8MB but climbs up until around 12MB and then it stops increasing. If I do the tweaks, it immediately starts at 12MB but won't increase anymore. I don't know why that is happening but it seems to match what you said :smiley:

Does that happen to you too @krazzei?

krazzei commented 5 years ago

Sorry, just had time to look at this. Yeah Adding runtime.GC() stops the memory form getting out of control, but that feels like a band aid and I wouldn't want to run the gc every frame.

And adding runtime.UnlockOSThread() and runtime.GOMAXPROCS(1) seems to have removed the allocations from the memory profile but in task manager (or activity monitor) the memory keeps increasing, not sure what that is.

In regards to the cevent pointer being the problem, I thought that was fixed in another ticket (I can't seem to find it right now). The solution was to move that cevent variable from the function and make it a package global variable.

flga commented 5 years ago

@krazzei you do not need to call GC manually, it was only to prove a point, go will do it for you. Leaks are not collected, if after GC the retained size is the same, there's no leak.

Try to run your program for a long period of time (5 mins for example) and see if the growth is unbounded or if it stabilizes eventually.

A good way to stress it is to move your mouse. It will probably be ok :)