Do we have memory leak issues?

[PhilippMDoerner]

Heyho,

Due to my concerns with the Video widget and the memory leaks with it I started looking a bit into debugging memory leaks.
Not that I am really any good at that yet, I'm stabbing in the dark.

Anyway, I stumbled into heaptrack, installed it and then I ran it against scale.nim to get a baseline, as I assumed scale.nim to be pretty stable in terms of memory consumption. I get heaptrack stats like this:

Which leads me to my questions:

• Do GtkWidgets get g_object_unref'd if they get destroyed ? I only saw the pointer for App getting unref'd in owlkettle.nim, but even that is something that happens before the application starts.
• Is this something that should be acted upon (I can barely interpret these graphs tbh, still trying to get an understanding) ?

I'm particularly uncertain on the last part because I'm half convinced that is just because the program closed and not an actual issue, but I really don't have any good amount of low-level understanding here.

Instructions to replicate:

• Install Heaptrack (I'm on arch where it's in the main repo)
• Compile the scale example with nim c --path:. -d:useMalloc --stacktrace:off --debugger:native --passC:-fno-omit-frame-pointer ./examples/widgets/scale.nim
• Start the example with heaptrack: heaptrack examples/widgets/scale
• Go to the "Top Down" tab, sort by "leaked", open the call-tree until you see brew (easiest way to do so: keep right arrow button pressed for a second)

[can-lehmann]

I am under the impression that GTK widgets do not need to be unrefed, as they are automatically destroyed upon being removed from the widget tree. As we only create new GTK widgets in Widget.build, which is only ever called from other widgets where the newly created widget is immediately inserted into the GUI tree (or from brew/open which should take care of memory), this should be fine.

Did you already try adding a suppression file for GTK4 as suggested here?

Edit: Sorry for the delays with the reviews, I was unexpectedly quite busy IRL. I hope to review some things later today.

[PhilippMDoerner]

With heaptrack specifically I have tried but was unsuccessful in removing the memory leak around brew.
I also managed to "prove" via g_object_weak_ref that the deallocation happens as you stated.
Minimal example for that for documentation purposes:

import std/[sequtils]
import owlkettle, owlkettle/[dataentries, playground, adw, widgetdef, common], owlkettle/bindings/gtk

viewable App:
  number: int

method view(app: AppState): Widget =
  result = gui:
    Window():
      title = "Scale Example"
      defaultSize = (800, 150)
      HeaderBar() {.addTitlebar.}:
        insert(app.toAutoFormMenu()) {.addRight.}
      
      Box(orient = OrientY):
        Box() {.expand: false.}:
          Button():
            icon = "list-add"
            style = [ButtonSuggested]
            proc clicked() =
              app.number.inc
          
          Button():
            icon = "user-trash-symbolic"
            style = [ButtonDestructive]
            proc clicked() =
              app.number.dec 
          
        for x in 0..app.number: 
          Label(text = "Label: " & $x)

adw.brew(gui(App()))

Then also change the beforeBuild hook of Label in widgets.nim to this:

  hooks:
    beforeBuild:
      state.internalWidget = gtk_label_new("")
      proc echoPtr(data, oldObjectPtr: pointer) {.cdecl.} =
        echo "Deallocating: ", data.repr
      g_object_weak_ref(state.internalWidget.pointer, echoPtr, state.internalWidget.pointer)

This will echo out when deallocating the label and you will see that echo whenever a label is removed.

I'm starting to believe the entire brew showing up in valgrind is a false positive for similar reasons as outlined in the SO question:
It is being relied on that upon closing the application the memory (of e.g. all the currently displayed Widgets) gets de-allocated, which strictly speaking is a memory leak but not one that matters.

So where I'm at is that valgrind and heaptrack for our purposes are useless regarding memory safety checking.
Or at least they are for me because I utterly fail at reading them, the vibes I'm getting is that you need some understanding of C and the

Sidenote:
I actually played around a bit with a video example that statically swaps back and forth between the same 2 video files.
It may not leak as much as I originally got the feeling that it did.

The following memory-amounts were "measured" by looking at the RAM consumption of the process with Gnome's System-Monitor application.

On my machine this example starts at ~44MB and catapults to around ~70MB after feeding it a 0.24MB video file.
Jumping to a second one increases the memory consumption, but pressing "reset" brings you back roughly to 60MB, maybe 0.1MB more. That 60MB is the "floor" for the reset state and it barely goes up, if at all.
As such, I get the impression that my gut feeling of "strong memory leaks" with the media stream API that I contributed may have been overblown.

The example:

const path1 = "/home/philipp/Videos/Screencasts/toast.webm"
const path2 = "/home/philipp/Videos/Screencasts/test.webm"

import owlkettle, owlkettle/[dataentries, playground, adw]
import owlkettle/bindings/gtk
viewable App:
  fileName: string
  mediaStream: MediaStream

method view(app: AppState): Widget =
  result = gui:
    Window():
      defaultSize = (1000, 600)
      title = "Video Example"
      
      HeaderBar() {.addTitlebar.}:
        WindowTitle {.addTitle.}:
          title = "Video Example"
          subtitle = app.filename
        
        Button {.addLeft.}:
          text = "Open1"
          style = [ButtonSuggested]
          
          proc clicked() =
            app.mediaStream = newMediaStream(path1)
            echo "Allocating: ", (app.mediaStream.gtk.pointer).repr
            proc echoPtr(data, oldObjectPtr: pointer) {.cdecl.} =
              echo "Deallocating: ", data.repr, " - ", oldObjectPtr.repr
            g_object_weak_ref(app.mediaStream.gtk.pointer, echoPtr, app.mediaStream.gtk.pointer)

        Button {.addLeft.}:
          text = "Open2"
          style = [ButtonSuggested]
          
          proc clicked() =
            app.mediaStream = newMediaStream(path2)
            echo "Allocating: ", (app.mediaStream.gtk.pointer).repr
            proc echoPtr(data, oldObjectPtr: pointer) {.cdecl.} =
              echo "Deallocating: ", data.repr, " - ", oldObjectPtr.repr
            g_object_weak_ref(app.mediaStream.gtk.pointer, echoPtr, app.mediaStream.gtk.pointer)

          
        Button {.addLeft.}:
          text = "Reset"
          style = [ButtonFlat]
          
          proc clicked() =
            app.mediaStream = nil
      
      Box(orient = OrientY):
        if not app.mediaStream.isNil():
          Video:
            mediaStream = app.mediaStream
        else:
          Label(text = "No file selected")

adw.brew(gui(App()))