oprypin / crsfml

Crystal bindings to SFML multimedia/game library
https://oprypin.github.io/crsfml
zlib License
350 stars 14 forks source link

Memory leak (maybe directly in sfml) #37

Closed wolfgang371 closed 3 years ago

wolfgang371 commented 4 years ago

Hi Oleh,

if I run below code on my machine it starts swapping pretty fast. Of course I understand the code is badly wrong; I construct a texture in the event loop and run it with 50 fps. Nevertheless I assume constructing textures in the loop is allowed and needed; and having it done less frequently doesn't change the problem.

Thanks for having a look!

require "crsfml"

window = SF::RenderWindow.new(SF::VideoMode.new(800, 600), "MyTest")
window.framerate_limit = 50

while window.open?
  while event = window.poll_event()
    if (
      event.is_a?(SF::Event::Closed) ||
      (event.is_a?(SF::Event::KeyPressed) && event.code.escape?)
    )
      window.close()
    end
  end

  window.clear SF::Color::Black
  texture = SF::RenderTexture.new(200,100) # define size (in texture)
  #~ texture.clear(SF::Color::Red)
  texture.draw(SF::Sprite.new(SF::Texture.from_file("resources/background.jpg"))) # leaks!
  texture.display()
  sprite = SF::Sprite.new(texture.texture) # make sprite out of texture
  sprite.position = {200, 100} # define position (in sprite)
  window.draw(sprite) # now draw to window

  window.display() # also consumes time until framerate is reached
end
oprypin commented 4 years ago

I assume constructing textures in the loop is allowed and needed

That is not really true, actually! I think constructing a texture constitutes pushing to GPU memory, which is expensive.

But probably a valid issue regardless, yes

oprypin commented 4 years ago

For me this program's RAM usage peaked at 900 MB. Seems OK.

wolfgang371 commented 4 years ago

hm. just double checked, I use SFML 2.5.1, seems still to be the latest. If I let it run for ~20s it has eaten more than 3GB on my Ubuntu (18.04.5) and still grows constantly...

oprypin commented 4 years ago
diff --git a/src/graphics/obj.cr b/src/graphics/obj.cr
index 3b2694b16..6de6ec232 100644
--- a/src/graphics/obj.cr
+++ b/src/graphics/obj.cr
@@ -3039,6 +3039,7 @@ module SF
   #
   # *See also:* `SF::Sprite`, `SF::Image`, `SF::RenderTexture`
   class Texture
+    @@n = 0
     @this : Void*
     # Types of texture coordinates that can be used for rendering
     enum CoordinateType
@@ -3052,11 +3053,13 @@ module SF
     #
     # Creates an empty texture.
     def initialize()
+      print "\r#{@@n += 1}   "
       SFMLExt.sfml_texture_allocate(out @this)
       SFMLExt.sfml_texture_initialize(to_unsafe)
     end
     # Destructor
     def finalize()
+      print "\r#{@@n -= 1}   "
       SFMLExt.sfml_texture_finalize(to_unsafe)
       SFMLExt.sfml_texture_free(@this)
     end

I applied this diff, and just ran your example with crystal run.

The number definitely stabilizes around 300.

wolfgang371 commented 4 years ago

I cannot tell what's wrong here: SFML-2.5.1-Compiled, crsfml (2.5.2), applied your instrumentation, run it with crystal run leak.cr like you do. I load the original background.jpg (53507 bytes), and both the number and the memory usage goes up; I terminate the process at 1000.

oprypin commented 4 years ago

So you say it's just Ubuntu...

Which version of Crystal? What do you mean by "SFML-2.5.1-Compiled"?

I'll try to run this on Ubuntu.

I'm on Arch Linux

wolfgang371 commented 4 years ago

Crystal 0.35.1 sorry, it seems I named the target folder "SFML-2.5.1-Compiled"; it's already a couple of months ago I compiled it.

oprypin commented 4 years ago

Yep, I confirm your finding.

oprypin commented 4 years ago

The easiest repro so far is this one

require "crsfml"

loop do
  t = SF::Texture.from_file("resources/background.jpg")
  sleep 0.01
end

But even this one stabilizes under 1.5 GiB on my Arch.

oprypin commented 4 years ago

I have concluded: no, it's just the garbage collector's "fault". It simply doesn't bother collecting the memory. If you just add a GC.collect call anywhere in the loop, then it actually works and everything is fine.

An actual memory leak wouldn't allow this collection to happen.

And I further conclude that there's nothing to do on my side.

First off, I could try to remove finalizers from my class, maybe that would make GC's job easier. But actually I can't do that, obviously the indirect memory for texture data needs to be released so there's no way around it.

And then, in terms of double-checking that there aren't any circular references -- well, I did, and it was very easy since the problem reproduces literally with just the texture creation.

oprypin commented 4 years ago

But as I was writing that, I realized a very likely explanation for why it's such a special case. Perhaps after all I can't just say that the GC is stupid.

So this texture data is a very big (comparatively) memory allocation that is done by SFML and so not tracked by GC. Perhaps GC is looking only at the tiny data bag that is the texture "metadata" struct and concludes that those ~1000 small allocations are completely fine to not collect yet, while the real memory hogs are allowed to build up.

So maybe this is worth looking into.

oprypin commented 4 years ago

With all of this said, yes, of course, please don't create texture objects in a tight loop, and you'll probably be fine :) It really is a special worst case scenario. Thanks for reporting, though.

oprypin commented 4 years ago

To elaborate: in almost all cases you'll create a texture of a given size once, and then only clear it in a loop if needed.

wolfgang371 commented 4 years ago

Thanks a lot for the quick analysis! I can confirm the GC.collect 'patch'. Seems like this strange OS specific behaviour of the GC is related to the different Crystal binaries/distros then... You probably want to close since it's not related to your lib...

wolfgang371 commented 4 years ago

BTW: the image is 800x600; if we assume 32bit internal representation, this would account to ~2GB after 1000 'iterations', so not too far off. Definitely big enough if the GC would consider not only the handles on Ubuntu ;)