golang-ui / nuklear

This project provides Go bindings for nuklear.h — a small ANSI C GUI library.
https://github.com/vurtun/nuklear
MIT License
1.57k stars 98 forks source link

Memory leak in glfw_gl3 NkPlatformRender #22

Closed robert-nix closed 6 years ago

robert-nix commented 7 years ago

Structures inside ConvertConfig are allocated but not freed by PassRef. Here's a diff of what I needed to modify in the generated code to stop the memory leak:

diff --git a/nk/cgo_helpers.go b/nk/cgo_helpers.go
index a1694bb..0f56276 100644
--- a/nk/cgo_helpers.go
+++ b/nk/cgo_helpers.go
@@ -2754,7 +2754,9 @@ func unpackSDrawVertexLayoutElement(x []DrawVertexLayoutElement) (unpacked *C.st
    }
    v0 := *(*[]C.struct_nk_draw_vertex_layout_element)(unsafe.Pointer(h0))
    for i0 := range x {
-       v0[i0], _ = x[i0].PassValue()
+       var valueAllocs *cgoAllocMap
+       v0[i0], valueAllocs = x[i0].PassValue()
+       allocs.Borrow(valueAllocs)
    }
    h := (*sliceHeader)(unsafe.Pointer(&v0))
    unpacked = (*C.struct_nk_draw_vertex_layout_element)(unsafe.Pointer(h.Data))
@@ -2809,6 +2811,7 @@ func (x *ConvertConfig) PassRef() (*C.struct_nk_convert_config, *cgoAllocMap) {
    mem82bf4c25 := allocStructNkConvertConfigMemory(1)
    ref82bf4c25 := (*C.struct_nk_convert_config)(mem82bf4c25)
    allocs82bf4c25 := new(cgoAllocMap)
+   allocs82bf4c25.Add(mem82bf4c25)
    var cglobal_alpha_allocs *cgoAllocMap
    ref82bf4c25.global_alpha, cglobal_alpha_allocs = (C.float)(x.GlobalAlpha), cgoAllocsUnknown
    allocs82bf4c25.Borrow(cglobal_alpha_allocs)
@@ -3105,6 +3108,7 @@ func (x *DrawVertexLayoutElement) PassRef() (*C.struct_nk_draw_vertex_layout_ele
    memeb0614d6 := allocStructNkDrawVertexLayoutElementMemory(1)
    refeb0614d6 := (*C.struct_nk_draw_vertex_layout_element)(memeb0614d6)
    allocseb0614d6 := new(cgoAllocMap)
+   allocseb0614d6.Add(memeb0614d6)
    var cattribute_allocs *cgoAllocMap
    refeb0614d6.attribute, cattribute_allocs = (C.enum_nk_draw_vertex_layout_attribute)(x.Attribute), cgoAllocsUnknown
    allocseb0614d6.Borrow(cattribute_allocs)
diff --git a/nk/impl_glfw_gl3.go b/nk/impl_glfw_gl3.go
index 04981b6..8db6621 100644
--- a/nk/impl_glfw_gl3.go
+++ b/nk/impl_glfw_gl3.go
@@ -97,6 +97,9 @@ func NkPlatformRender(aa AntiAliasing, maxVertexBuffer, maxElementBuffer int) {
        NkBufferInitFixed(vbuf, vertices, Size(maxVertexBuffer))
        NkBufferInitFixed(ebuf, elements, Size(maxElementBuffer))
        NkConvert(state.ctx, dev.cmds, vbuf, ebuf, config)
+       vbuf.Free()
+       ebuf.Free()
+       config.Free()

        gl.UnmapBuffer(gl.ARRAY_BUFFER)
        gl.UnmapBuffer(gl.ELEMENT_ARRAY_BUFFER)
xlab commented 7 years ago

Thanks @Mischanix ! I think we should split this into two parts, the first part is easy (adding explicit Free in NkPlatformRender). The second part is harder, because it involves cgo_helpers.go which are automatically generated and should not be modified by hand. So I need to change c-for-go behaviour to respect that.

If you'd provide a small test program so I could see the leak, that would help a lot!

robert-nix commented 7 years ago

I've hacked in a little heap size routine for debugging on Windows: https://github.com/Mischanix/nuklear/commit/35b983c73a941c661d5f335f242f8437fe77830f.

I'm compiling this with mingw-w64 GCC from MSYS2. Using mallinfo may be a suitable alternative under linux.

So, in github.com/Mischanix/nuklear/cmd/nk-example on Windows there is now a label showing that the heap is growing steadily from frame to frame.

I have put the manual fixes in a separate branch: https://github.com/golang-ui/nuklear/compare/master...Mischanix:manually-fix-memory-leaks?expand=1

xlab commented 7 years ago

@Mischanix your help is invaluable, thank you so much! I will use this example to figure out how to improve c-for-go to produce leak-free code for this.