Skip to content

Conversation

@peat-psuwit
Copy link
Contributor

@peat-psuwit peat-psuwit commented Jul 21, 2025

Instead of storing pointer to GObject directly in JS Object, store a
C++ GC-able wrapper class.

Replace a Persistent in Closure with a TracedReference. Then, keep track
of created Closure in GObjectWrapper. When GObjectWrapper is traced by
cppgc, trace those reference to tell cppgc they're still alive.

By not creating a Persistent, we prevent a reference loop where the
Persistent holds function, holds JS wrapper, holds GObject, holds
Persistent. When TracedReference, evetually the whole loop will not be
traced by cppgc, allowing the whole loop to be dropped.

This approach is inspired by PyGObject and utilize V8's Oilpan (also called cppgc), C++ garbage collector integrated with V8. This, however, means cutting support for NodeJS older than version 20.6. That said, NodeJS 18 has just became end-of-life (not counting commercial support), so maybe it's not that bad, compared to not leaking memory?


TODO:

  • Test with Node 23+, where mechanics of wrapping GC-able C++ inside V8 Object changes. Node 23+ itself requires other changes before Node-GTK compiles. See Update to be node 24 compatible; bump nan and node-gyp dependency version #374.
  • Re-organize the code to move out-of-place function declaration in gobject.cc (see FIXME in the code). Also consider moving GObjectWrapper class declaration to gobject.h.
  • Maybe also track & trace callbacks and vfuncs?

TODO: test on Node 23+. Node-GTK itself requires update. Has PR.

Instead of storing pointer to GObject directly in JS Object, store a
C++ GC-able wrapper class. This enables us to track JS memory embedded
inside GObject, avoiding reference loop.
Replace a Persistent in Closure with a TracedReference. Then, keep track
of created Closure in GObjectWrapper. When GObjectWrapper is traced by
cppgc, trace those reference to tell cppgc they're still alive.

By not creating a Persistent, we prevent a reference loop where the
Persistent holds function, holds JS wrapper, holds GObject, holds
Persistent. When TracedReference, evetually the whole loop will not be
traced by cppgc, allowing the whole loop to be dropped.

This approach is inspired by PyGObject.
Copy link
Owner

@romgrk romgrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good approach and I'd be happy to merge it if we can make it work. This project hasn't received enough love lately, and I haven't done much work to keep it working with recent versions of nodejs. I'm fine with dropping older versions, but the CI will need attention to keep tests passing on at least one version. Let's see if we can get #374 merged first.

@romgrk
Copy link
Owner

romgrk commented Jul 27, 2025

Looks like #374 could be merged soon.

Test with Node 23+, where mechanics of wrapping GC-able C++ inside V8 Object changes

I haven't followed V8 development lately, do you have a link to the changelog/docs for that? Do you expect any big issue with compat between <23 and 23+?

@peat-psuwit
Copy link
Contributor Author

I haven't followed V8 development lately, do you have a link to the changelog/docs for that? Do you expect any big issue with compat between <23 and 23+?

I can't find doc, but the relevant commit is: nodejs/node@0be79f4

NodeJS actually gives an API for wrapping part (and that part technically have compatibility layer). However I can't find an equivalent API for unwrapping from NodeJS and has to directly use V8 APIs.

I've already written code for that and put #if NODE_VERSION_AT_LEAST(23,0,0) around affected code (2 of them). It's just that I haven't tested if my code actually functions.

@romgrk
Copy link
Owner

romgrk commented Jul 28, 2025

#374 was merged and I'll release it as v1.0.0 soon. Afaiu, this is all internal and wouldn't break the external API so it can be released as a minor/patch once it's ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants