mosra / magnum

Lightweight and modular C++11 graphics middleware for games and data visualization
https://magnum.graphics/
Other
4.74k stars 439 forks source link

Memory leak when destroying GL::Mesh #632

Closed bz-next closed 5 months ago

bz-next commented 5 months ago

There may be something I am not understanding properly about using the library, but I've found that GL::Mesh doesn't seem to clear memory associated with the mesh when it is destroyed.

As a test, I wrote the following code in a loop:

    for (int i = 0; i < 1000000; ++i) {
        GL::Mesh test = MeshTools::compile(Primitives::cubeSolid());
    }

From what I can tell, when the mesh goes out of scope, it should free any memory that it is using, but this example uses around 5GiB of memory on my machine.

Update: I've also tested this by adding the above code to the primitives example and the same thing happens: a lot of persistent memory use.

mosra commented 5 months ago

Hi, thanks a lot for the report and the to-the-point repro case!

This is unfortunately something that recently slipped through when doing a relatively large cleanup of mesh internals in f7a6d79aa07cb81c6170d7ab98d0a01856f3f16a (on Nov 22, 2023), and you're the first to notice. I checked with apitrace and it was the VAO not getting destroyed properly but the index/vertex buffers were, so it's probably the GL spec mandating they're kept for as long as any VAO referencing them, or something, causing the GPU memory usage endlessly growing.

In any case, this should be fixed with b1ba1f076d3e8b4295b1afac94e95ff8a846e619 (in the next branch). Can you confirm? Once you do, I'll push that commit to master.

Oh, and I subscribed to your project, will be watching closely (if that's okay, haha) :+1:

bz-next commented 5 months ago

Appears fixed, thank you for the quick response!

I first noticed this bug when re-compiling meshes for a game map with a few million vertices, it'd cause a leak of a few hundred MiB per recompile. Now, it's clear that the memory is being freed, and memory use is consistent.

Always glad to have another watcher :) Thanks for providing this super slick library!

mosra commented 5 months ago

Awesome, thanks for confirmation -- the commit is in master now.

For a mesh that has its contents modified very often but the layout stays the same, MeshTools::compile() (and MeshData in general) might be a bit of an overkill, a simpler approach would be to have just some Containers::Array<Vertex> that's repopulated and reuploaded to an already existing vertex / index buffer, together with adjusting setCount() on the mesh if needed.

Or, alternatively, depending on how the inputs to the map recompiling are structured and how generic they are, you could store Trade::MeshData instances for the parts and then pass them all to MeshTools::concatenate(). That won't avoid the GL object recreation per se, but could at least simplify the overall logic and make it easier to modify / extend later (for example if you'd ever need additional attributes like vertex color, or would want to pack them for smaller memory use etc.).

GL object recreation could then be avoided by creating the vertex/index buffers explicitly with this compile() overload and subsequently just calling setData() on those without creating them anew, and (assuming the data layout didn't change) calling just mesh.setCount(meshData.indexCount()) on the pre-existing mesh instead of the whole compile() operation again.

But also, if this is an operation that isn't done that often and the data size isn't too massive, the existing way is probably fine and won't hurt perf. So I'm just listing the possibilities here :) Have fun!