Update `=destroy` hooks

[can-lehmann]

With Nim 2.0.0, the function signature for =destroy hooks changed. These need to be updated, the compiler warnings should tell you the relevant locations.

[can-lehmann]

@PhilippMDoerner do you want me to assign you to this?

[PhilippMDoerner]

Yeah, might make for good research and when I'm too bored at the prospect of updating docs of my mapster package this might be a good fit

[PhilippMDoerner]

From my understanding this only affects 2 places of code: 1) PixiBuf in widgets.nim around line ~460

  PixbufObj* = object
    gdk: GdkPixbuf

  PixBuf* = ref PixbufObj

proc `=destroy`(x: PixbufObj) =
  echo "PixbufObj destructor"
  g_object_unref(pointer(x.gdk))

proc newPixbuf(gdk: GdkPixbuf): Pixbuf =
  if gdk.isNil:
    raise newException(ValueError, "Unable to create Pixbuf from GdkPixbuf(nil)")
  new(result)
  result.gdk = gdk

2) TextBufferObj in widgets.nim around line ~2000

proc `=destroy`(buffer: TextBufferObj) =
  echo "DESTRUCTOR TEXT"
  g_object_unref(pointer(buffer.gtk))

proc newTextBuffer*(): TextBuffer =
  new(result)
  result.gtk = gtk_text_buffer_new(nil.GtkTextTagTable)

You can test 1) by compiling and playing around with examples/widgets/picture.nim You'll be able to see how it triggers the echo.

No clue about TextBufferObj as of now and what else would need to happen to avoid memory leaks in the case of exceptions occurring during destruction.

[can-lehmann]

Looks good so far, I am just going to trust that you tested it :) The text buffer should be testable using the text view example.

If it fixes the warnings, it should also close this issue. I don't see anything about recoverable errors in the g_object_unref docs, so that should be fine. The minimum Nim version requirement in the nimble file (and the workflows) needs to be raised to 2.0.0, because I don't think that previous Nim versions will execute these destructors. If you want you could also add the destructor for Stylesheet in the same PR (https://github.com/can-lehmann/owlkettle/issues/76).

[PhilippMDoerner]

I tested only PixBufObj so far given I haven't found an example to compile where textbuffer is used. I'll test that of course as well once I find an example.

I am however also double and triple checking together with PMunch to see whether there's anything else that could possibly be overlooked given my low understanding of destructors and memory allocation. Something we stumbled over was that strictly speaking one could just remove "ref" and have PixBuf follow value-type semantics, given that it's just a pointer (or in the current version: A pointer/ref to a pointer of a GTK object).

So this here seems functional:

  Pixbuf* = object
    gdk: GdkPixbuf

proc `=destroy`(x: Pixbuf) =
  g_object_unref(pointer(x.gdk))

I'm not sure what/how much that would break though. I can only state that compiling nim r ./examples/widgets/picture.nim works what seems like the same as before.

Given that I haven't understood the mechanics under the hood in any amount of depth I can't evaluate if that is or isn't dangerous. Given the semantics change I can only predict that it does change some logic somewhere with copy behaviour for PixBuf when one uses it, but no clue what implications that has. Should we, to keep the entire logic and semantics the same for now, stick with introducing the intermediary PixBufObj?

[can-lehmann]

I like the option of dropping the ref. However you are going to need to add a =copy hook, otherwise this wont compile:

let pixbuf = loadPixbuf(path)
app.pixbuf = pixbuf
echo pixbuf.height

So you want this:

proc `=destroy`*(pixbuf: Pixbuf) =
  if not isNil(pixbuf.gdk):
    g_object_unref(pointer(pixbuf.gdk))
    echo "destroy"

proc `=copy`*(a: var Pixbuf, b: Pixbuf) =
  if pointer(a.gdk) != pointer(b.gdk):
    `=destroy`(a)
    wasMoved(a)
    echo "copy"
    if not isNil(b.gdk):
      g_object_ref(pointer(b.gdk))
    a.gdk = b.gdk

I would also recommend adding weak_refs for testing that the GdkObjects actually get deleted.

proc notify(data, oldObjectPtr: pointer) {.cdecl.} =
  echo "delete pixbuf"
g_object_weak_ref(pointer(gdk), notify, nil)

Here is my complete test setup:

diff --git a/examples/widgets/picture.nim b/examples/widgets/picture.nim
index 277073c..935eb63 100644
--- a/examples/widgets/picture.nim
+++ b/examples/widgets/picture.nim
@@ -75,7 +75,9 @@ method view(app: AppState): Widget =

                 proc callback(pixbuf: Future[Pixbuf]) =
                   app.loading = false
-                  app.pixbuf = pixbuf.read()
+                  let pixbuf = pixbuf.read()
+                  app.pixbuf = pixbuf
+                  echo pixbuf.height
                   discard app.redraw()

                 future.addCallback(callback)
@@ -84,10 +86,12 @@ method view(app: AppState): Widget =
               else:
                 # Load sync
                 try:
-                  app.pixbuf = loadPixbuf(path)
+                  let pixbuf = loadPixbuf(path)
+                  app.pixbuf = pixbuf
+                  echo pixbuf.height
                 except IoError as err:
                   echo err.msg
-                  app.pixbuf = nil
+                  #app.pixbuf = newPixbuf(nil)

         Button {.addLeft.}:
           text = "Save"
diff --git a/owlkettle/gtk.nim b/owlkettle/gtk.nim
index da3baa2..56545ca 100644
--- a/owlkettle/gtk.nim
+++ b/owlkettle/gtk.nim
@@ -340,6 +340,7 @@ proc g_signal_connect_data*(widget: pointer,
 proc g_type_from_name*(name: cstring): GType
 proc g_object_new*(typ: GType): pointer {.varargs.}
 proc g_object_ref*(obj: pointer)
+proc g_object_weak_ref*(obj: pointer, notify: proc (data, oldObjectPtr: pointer) {.cdecl.}, data: pointer)
 proc g_object_unref*(obj: pointer)
 proc g_object_set_property*(obj: pointer, name: cstring, value: ptr GValue)
 proc g_type_fundamental*(id: GType): GType
diff --git a/owlkettle/widgets.nim b/owlkettle/widgets.nim
index 4523f00..6a8233b 100644
--- a/owlkettle/widgets.nim
+++ b/owlkettle/widgets.nim
@@ -458,17 +458,33 @@ type
   Colorspace* = enum
     ColorspaceRgb

-  Pixbuf* = ref object
+  Pixbuf* = object
     gdk: GdkPixbuf

-proc finalizer(pixbuf: Pixbuf) =
-  g_object_unref(pointer(pixbuf.gdk))
+proc `=destroy`*(pixbuf: Pixbuf) =
+  if not isNil(pixbuf.gdk):
+    g_object_unref(pointer(pixbuf.gdk))
+    echo "destroy"
+
+proc `=copy`*(a: var Pixbuf, b: Pixbuf) =
+  if pointer(a.gdk) != pointer(b.gdk):
+    `=destroy`(a)
+    wasMoved(a)
+    echo "copy"
+    if not isNil(b.gdk):
+      g_object_ref(pointer(b.gdk))
+    a.gdk = b.gdk
+
+proc isNil*(pixbuf: Pixbuf): bool = pixbuf.gdk.isNil()
+proc `==`*(a, b: Pixbuf): bool = pointer(a.gdk) == pointer(b.gdk)

 proc newPixbuf(gdk: GdkPixbuf): Pixbuf =
   if gdk.isNil:
     raise newException(ValueError, "Unable to create Pixbuf from GdkPixbuf(nil)")
-  new(result, finalizer=finalizer)
-  result.gdk = gdk
+  result = Pixbuf(gdk: gdk)
+  proc notify(data, oldObjectPtr: pointer) {.cdecl.} =
+    echo "delete pixbuf"
+  g_object_weak_ref(pointer(gdk), notify, nil)

 proc newPixbuf*(width, height: int,
                 bitsPerSample: int = 8,
@@ -549,7 +565,7 @@ proc handlePixbufReady(stream: pointer, result: GAsyncResult, data: pointer) {.c
   var error = GError(nil)
   let pixbuf = gdk_pixbuf_new_from_stream_finish(result, error.addr)
   if error.isNil:
-    future.complete(Pixbuf(gdk: pixbuf))
+    future.complete(newPixbuf(pixbuf))
   else:
     let message = $error[].message
     future.fail(newException(IoError, "Unable to load pixbuf: " & message))

[can-lehmann]

I was not able to get this working with distinct instead of object yet, maybe you could look into that, as it would allow us to keep supporting nil values (both for better backward compatibility & since Nim is not particularly great at non nil types).

[can-lehmann]

Also, if we move to value semantics, I would also add them for CairoContext (https://github.com/can-lehmann/owlkettle/blob/main/owlkettle/cairo.nim#L28), so that you do not need to do manual memory management for it (calling the destroy proc) anymore.

[PhilippMDoerner]

So basically it makes a whole bunch of things easier/streamlines them which means its worthwhile and a learning experience, which means the only right choice is to dig into it as far as I can see

[can-lehmann]

Yes, I think that if arc/orc had been the default when this code was written, it would have been the correct architectural choice. Since it is the default now, we should probably adopt this (= value semantics) design.

[PhilippMDoerner]

I'm looking through the copy-hook atm: Why destroy the original ref on copy? I would anticipate after an assignment that the previous reference was still available, why go against that ? Or is this basically trying to establish ref semantics on the value-type Pixbuf?

Also what does weakref even do?

[can-lehmann]

WeakRef notifies you when the object is deleted. So you can use it to check that you implemented everything correctly (Once there are no more references, the Pixbuf/... should be deleted & the weak ref notifies you that that happened). https://developer-old.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#GWeakNotify

[can-lehmann]

=copy is called when an assignment happens (a = b) in this case the reference count of the thing originally stored in a should be decreased, since it is no longer referenced by the variable a. But thing stored in b is now also referred to by a, so we need to increase the reference count. So we call =destroy and g_object_ref. https://nim-lang.org/docs/destructors.html#lifetimeminustracking-hooks-nimeqcopy-hook

[PhilippMDoerner]

I've amassed enough understanding now that I managed to get the example running with a bit of my own setup. I'm a bit confused as to the operations going on. These are my widget changes:

  Pixbuf* = object
    gdk: GdkPixbuf

proc isNil*(pixbuf: Pixbuf): bool = pixbuf.gdk.isNil()
proc `==`*(a, b: Pixbuf): bool = pointer(a.gdk) == pointer(b.gdk)

proc `=destroy`(pixbuf: Pixbuf) =
  if isNil(pixbuf.gdk):
    echo "Destructor call on pixbuf with nil"
    return

  echo "Destructor: Remove ref ", cast[int](pixbuf.gdk)
  g_object_unref(pointer(pixbuf.gdk))

proc `=copy`*(dest: var Pixbuf, source: Pixbuf) =
  let areSamePixbuf = pointer(source.gdk) == pointer(dest.gdk)
  if areSamePixbuf:
    echo "copy call on pixbuf with nil"
    return

  echo "copy source --> dest: ", cast[int](source.gdk), " --> ", cast[int](dest.gdk)
  `=destroy`(dest)
  wasMoved(dest)
  if not isNil(source.gdk):
    echo "- Add ref ", cast[int](source.gdk)
    g_object_ref(pointer(source.gdk))

  dest.gdk = source.gdk

proc newPixbuf(gdk: GdkPixbuf): Pixbuf =
  if gdk.isNil:
    raise newException(ValueError, "Unable to create Pixbuf from GdkPixbuf(nil)")
  result = Pixbuf()
  result.gdk = gdk

  # TODO: Remove
  proc notify(data, oldObjectPtr: pointer) {.cdecl.} =
    echo "delete pixbuf ", cast[int](oldObjectPtr)
  g_object_weak_ref(pointer(gdk), notify, nil)

in pictures.nim I just commented out app.pixbuf = nil same as you did.

This is what I logged (I split the echo'd messages up by hand, one copy leads to a copy-echo as well as a destructor-echo as it triggers the destructor once, as well as an add-ref echo.:

Logs:
Initial Loading:
  Destructor call on pixbuf with nil
  Destructor call on pixbuf with nil

After loading img:
  Destructor call on pixbuf with nil

  copy source --> dest: 94548955859456 --> 0
  Destructor call on pixbuf with nil
  - Add ref 94548955859456

  copy source --> dest: 94548955859456 --> 0
  Destructor call on pixbuf with nil
  - Add ref 94548955859456

  Destructor: Remove ref 94548955859456

After rotate button click:
  Destructor: Remove ref 94548955859456

  copy source --> dest: 94548959159888 --> 0
  Destructor call on pixbuf with nil
  - Add ref 94548959159888

  copy source --> dest: 94548959159888 --> 94548955859456
  Destructor: Remove ref 94548955859456
  - Add ref 94548959159888

  Destructor: Remove ref 94548959159888
  delete pixbuf 94548955859456

I'm kinda confused why everything seems to be happening twice. We definitely do see a deletion of a pixbuf every mutation of the image though, so that seems... maybe correct? I dunno.

[can-lehmann]

Probably because it happens once in the App viewable and then in the Picture renderable.

[PhilippMDoerner]

As someone with little to no experience that low level I'll assume that this doing double duty is less terrifying than it would be in webdev if those were expensive db queries doubling up on a system under heavy load.

[can-lehmann]

Yes, its just a few pointer assignments and integer increments / decrements (basically free). It would be worse if those were actual copies of large data structures, but since its just pointers, it is not an issue.

[PhilippMDoerner]

If that's the case then I think we should be good with the value based semantics version for pixbuf.

I also tested what gets echo'd out of the ref-based semantics:

proc `=destroy`(pixbuf: PixbufObj) =
  if isNil(pixbuf.gdk):
    echo "Destructor call on pixbuf with nil"
    return

  echo "Destructor: Remove ref ", cast[int](pixbuf.gdk)
  g_object_unref(pointer(pixbuf.gdk))

proc newPixbuf(gdk: GdkPixbuf): Pixbuf =
  if gdk.isNil:
    raise newException(ValueError, "Unable to create Pixbuf from GdkPixbuf(nil)")
  new(result)
  result.gdk = gdk

  # TODO: Remove
  proc notify(data, oldObjectPtr: pointer) {.cdecl.} =
    echo "delete pixbuf ", cast[int](oldObjectPtr)
  g_object_weak_ref(pointer(gdk), notify, nil)

That logs this:

Logs:
Initial Loading:
After rotate button click:
  Destructor: Remove ref 94189444972880
  delete pixbuf 94189444972880

Seems like we might be missing some ref-copies since now the ref of PixbufObj gets copied around rather than PixbufObj itself. Given that I don't have a copy-proc here we're obviously not getting informed on when the ref of PixbufObj gets copied, so that's missing. From what I can see we might be making fewer unnecessary copies of the actual buffer this way, but given that you stated that that's cheap anyway I think we should be good.

[PhilippMDoerner]

I was not able to get this working with distinct instead

I could also be looking into this. What were you meaning when you stated "distinct"? Like type PixBuf = distinct GdkPixBuf ?

[can-lehmann]

Yes, that would be nice, since it allows for using nil values (Pixbuf(nil)).

[PhilippMDoerner]

If that's the main reason/benefit, couldn't you just have a constructor ?

proc initPixbuf(buffer: GdkPixBuf): Pixbuf = 
  Pixbuf(gdk: buffer)

let x = initPixbuf(nil)

[can-lehmann]

Sure, I just think this looks kind of weird :smile:

[PhilippMDoerner]

Okay, just tried it out. Apparently distinct pointer is allowed to have destructors for reasons I don't understand.

  Pixbuf* = distinct GdkPixBuf

proc isNil*(pixbuf: Pixbuf): bool = pixbuf.isNil()
proc `==`*(a, b: Pixbuf): bool = pointer(a) == pointer(b)

proc `=destroy`(pixbuf: Pixbuf) =
  if isNil(pixbuf):
    echo "Destructor call on pixbuf with nil"
    return

  echo "Destructor: Remove ref ", cast[int](pixbuf)
  g_object_unref(pointer(pixbuf))

proc `=copy`*(dest: var Pixbuf, source: Pixbuf) =
  let areSamePixbuf = pointer(source) == pointer(dest)
  if areSamePixbuf:
    echo "copy call on pixbuf with nil"
    return

  echo "copy source --> dest: ", cast[int](source), " --> ", cast[int](dest)
  `=destroy`(dest)
  wasMoved(dest)
  if not isNil(source):
    echo "- Add ref ", cast[int](source)
    g_object_ref(pointer(source))

  dest = source

proc newPixbuf(gdk: GdkPixbuf): Pixbuf =
  if gdk.isNil:
    raise newException(ValueError, "Unable to create Pixbuf from GdkPixbuf(nil)")
  result = gdk.Pixbuf

This worked for me. In the sense it compiled, the example started and I could select an image and rotate it. You mentioned troubles in your own attempts, what did they look like?

[can-lehmann]

If I remember correctly, it never recognized both hooks at the same time. The manual states that destructors + distinct should work, I just probably made some stupid mistake. Also, are you sure that dest = source does not cause an infinite loop?

[PhilippMDoerner]

I must have forgotten to save because I'm getting issues about it already having copy/destroy hooks implicitly created from within gtk.nim. Explicitly

proc g_value_new*(icon: GIcon): GValue =
  discard g_value_init(result.addr, g_type_from_name("GIcon"))
  g_value_set_object(result.addr, pointer(icon))

No idea what that was on about. For the most part it means I deleted the =delete and =copy hooks when trying to compile with the version where type Pixbuf = distinct GdkPixbuf

I got it to run, but it definitely leaks memory. By rotating the image 50 or so times I could watch my memory go from 2.7GB free to 2.3 GB free. The same issue does not occur when running with the version where Pixbuf is just an object type.

[can-lehmann]

copy/destroy hooks implicitly created from within gtk.nim

Yes, thats also the thing I ran into. As far as I can see, the code the nim compiler points you to is completely unrelated to any of this (but I also got pointed to the same section of gtk.nim).

[PhilippMDoerner]

I'd take that as a hint to either start debugging where those implicit copy/destroy hooks come from (which might be a compiler bug) or just use hooks on a wrapper value type for now.

Given that I'm already in very new territory (though at least I've gotten an understanding of macros over the last couple months) I'm tempted to leave it at value objects. Sound good?

[can-lehmann]

Ok, I think I might have figured it out. The issue is using the pointer(...) cast, so you need to do something like this (note the cast[...](...):

type
  Test = distinct pointer

proc `=destroy`(test: Test) =
  if cast[pointer](test).isNil:
    echo "destroy"

proc `=copy`(a: var Test, b: Test) =
  echo "copy"
  if cast[pointer](a) != cast[pointer](b):
    `=destroy`(a)
    wasMoved(a)
    cast[var pointer](a) = cast[pointer](b)

If you use a pointer(...) cast instead, you get the error we encountered before.

Also if I remember correctly, the =destroy and =copy hooks must be defined immediately after the type definition. So you cannot define isNil and == before defining the hooks.

[PhilippMDoerner]

I can't say I figured it out. The copy hook does eternal loop due to the pointer cast I believe and causes SOs:

  Pixbuf* = distinct GdkPixBuf

proc `=destroy`(pixbuf: Pixbuf) =
  if cast[pointer](pixbuf) == nil:
    return

  g_object_unref(cast[pointer](pixbuf))

proc `=copy`*(dest: var Pixbuf, source: Pixbuf) = # This is line 469
  let areSamePixbuf = cast[pointer](source) == cast[pointer](dest)
  if areSamePixbuf:
    return

  `=destroy`(dest) # This is line 474
  wasMoved(dest)
  if cast[pointer](source) != nil:
    g_object_ref(cast[pointer](source))

  dest = source

... in a whole bunch of later procs, find replace .gdk => .GdkPixBuf

Traceback (most recent call last) /home/philipp/dev/owlkettle/examples/widgets/picture.nim(170) picture /home/philipp/dev/owlkettle/owlkettle.nim(193) brew /home/philipp/dev/owlkettle/owlkettle/mainloop.nim(75) runMainloop /home/philipp/dev/owlkettle/owlkettle/widgetutils.nim(46) eventCallback /home/philipp/dev/owlkettle/owlkettle/widgetutils.nim(31) redraw /home/philipp/dev/owlkettle/owlkettle/widgetdef.nim(77) redraw /home/philipp/dev/owlkettle/owlkettle/widgetdef.nim(53) view /home/philipp/dev/owlkettle/owlkettle/widgets.nim(469) view /home/philipp/dev/owlkettle/owlkettle/widgets.nim(469) =copy ... same call 2000 times /home/philipp/dev/owlkettle/owlkettle/widgets.nim(474) =copy /home/philipp/dev/owlkettle/owlkettle/widgets.nim =destroy Error: call depth limit reached in a debug build (2000 function calls). You can change it with -d:nimCallDepthLimit= but really try to avoid deep recursions instead.

[can-lehmann]

You need to cast to pointer cast[var pointer](dest) = cast[pointer](source), otherwise it will be a recusive function.

[PhilippMDoerner]

Seems like the assignment is destined to segfault because dest is nil and nim just segfaults when I try to assign to it:

proc `=copy`*(dest: var Pixbuf, source: Pixbuf) =
  let areSamePixbuf = cast[pointer](source) == cast[pointer](dest)
  if areSamePixbuf:
    return

  `=destroy`(dest)
  wasMoved(dest)
  if cast[pointer](source) != nil:
    g_object_ref(cast[pointer](source))

  let val = cast[pointer](source)
  echo val.repr, " - ", dest.repr
  cast[var pointer](dest) = val

00005640DFB7F6B0 - nil  # The echo you can see right before the assignment, so dest is nil
Traceback (most recent call last)
/home/philipp/dev/owlkettle/examples/widgets/picture.nim(170) picture
/home/philipp/dev/owlkettle/owlkettle.nim(193) brew
/home/philipp/dev/owlkettle/owlkettle/mainloop.nim(75) runMainloop
/home/philipp/dev/owlkettle/owlkettle/widgetutils.nim(46) eventCallback
/home/philipp/dev/owlkettle/owlkettle/widgetutils.nim(31) redraw
/home/philipp/dev/owlkettle/owlkettle/widgetdef.nim(77) redraw
/home/philipp/dev/owlkettle/owlkettle/widgetdef.nim(53) view
/home/philipp/dev/owlkettle/owlkettle/widgets.nim(469) view
/home/philipp/dev/owlkettle/owlkettle/widgets.nim(481) =copy
SIGSEGV: Illegal storage access. (Attempt to read from nil?)

Similar inim experiment:

nim> type A = distinct pointer
nim> var x: A 
nim> echo x.repr
nil
nim> cast[var pointer](x)
Error: expression 'cast[var pointer](x)' is of type 'var pointer' and has to be used (or discarded); start of expression here: /tmp/inim_1694789970.nim(1, 1)
nim> var y: A
nim> cast[var pointer](x) = y.pointer
/tmp/inim_1694789970.nim(12) inim_1694789970
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
Error: execution of an external program failed: '/tmp/inim_1694789970'

[can-lehmann]

I guess you can just cast[ptr pointer](dest.addr)[] = cast[pointer](source)

[PhilippMDoerner]

Okay, that got the example to compile and run. However despite the destroy hook it still leaks memory like a sieve: After 5 seconds of dedicated tapping on rotate:

Edit: That is despite the destructor hook getting called. I added an echo and I could observe a destructor call for every time I tapped the button.

[can-lehmann]

Does the weak ref callback get called?

[PhilippMDoerner]

Good point! It does not! Which is weird because the echo I have is after the nil-check, so every echo means there definitely is a call to unref:

proc `=destroy`(pixbuf: Pixbuf) =
  if cast[pointer](pixbuf) == nil:
    return
  echo "Destroy me"
  g_object_unref(cast[pointer](pixbuf))

[can-lehmann]

Could you open a draft PR, so I can see the current code?