scorninpc / php-gtk3

GTK3 extension for PHP
https://andor.com.br/
GNU Lesser General Public License v3.0
113 stars 10 forks source link

Method "connect" leaks memory OR method "destroy" doesn't destroy all children #81

Closed ntfs1984 closed 1 month ago

ntfs1984 commented 7 months ago

Hello. Found that "connect" method leaks memory if applied to previously declared and even destroyed object.

Example.

<?php
Gtk::init();
function GtkWindowDestroy($widget=NULL, $event=NULL)
 {
 Gtk::main_quit();
 }

function refresh() 
 {
 global $win, $box, $button;
 $box->destroy();
 $button->destroy();    
 $box = new GtkBox(GtkOrientation::HORIZONTAL);
 $button = GtkButton::new_with_label(rand(1,100));
 $button->connect("clicked",function($button) {}); // Comment this to stop memory leak
 $box->add($button);
 $win->add($box);   
 $button->show();
 $box->show();
 echo "\n".round(memory_get_usage()/1024,2) . " Kb\n";
 } 

$win = new GtkWindow();
$win->set_default_size(300, 200);
$win->connect("destroy", "GtkWindowDestroy");
$box = new GtkBox(GtkOrientation::HORIZONTAL);
$button = GtkButton::new_with_label(rand(1,100));
$box->add($button);
$win->add($box);
Gtk::timeout_add(2000, function () {refresh();return true;});
$win->show_all();
Gtk::main();

This code leaks memory upon every iteration of timeout_add. If we comment line "$button->connect("clicked",function($button) {});" - memory stopped to leak, this means that g_signal_connect doesn't replace function within previously declared object, but add it.

Is there a way to fix such situation according example? Thanks.

scorninpc commented 7 months ago

Hey Hello!

As you can see on doc, destroy not free memory

widget will still point to valid memory, allowing you to release the references you hold, but you may not query the widget’s own state.

Look a nice reference too

gdk_destroy() just sends the signal to politely ask everyone to remove their references, but doesn't force any references to disappear. The issue was that I somehow misunderstood the effect of ref_sink(), which does NOT need to paired with a ref() call

Manage memory on GTK is not a simple task, as well the problem of clone objects

If you want a deep understand of Gtk memory, this a famous article

In any case, I dont see any use of recreate all stack of widgets every 2 seconds, as we can use global variables or so! This cause not only memory problem, but use of cpu and disk also

ntfs1984 commented 7 months ago

I'm still checking all possible solutions to create system tray for your dock.

I found way to do this over d-bus.

Any change of system tray state (for example incoming message to messenger) requires re-draw of system tray (like it used in X system tray apps) with new properties of applications.

So, recreation of stack performing not at every 2 sec, but at every changed state. Received message to Telegram - icon recreated, +2Kb of memory used. Have read message of Telegram - icon recreated, +2Kb of memory used.

But thanks for links, will dig how to resolve these memory leaks.

scorninpc commented 7 months ago

if you do successfully icon tray, i'd like to take a look and add this to cookbook. I know this is a UX features of all OS (windows, linux, mac) but I love systray too

ntfs1984 commented 7 months ago

Well, if shortly:

I'm not professional programmer, so not able to see all best methods, but you can use this snixembed app and d-feet utility to understand how it works.

scorninpc commented 7 months ago

ahhh very nice workaround!

ntfs1984 commented 6 months ago

Anyway want to re-open ticket just for your info. Step by step I found root of memory leak, and it's not GTK.

Function "Php::Value GObject_::connect_internal" of GObject.cpp contains some struct called "callback_object". This struct seems is not freeing before end of function.

It's not a critical bug, but you will 100% meet it once will develop some software with content refresh and big uptime, like dockStupido.

scorninpc commented 6 months ago

Oh! nice to know! I'll investigate if we can free this to prevent it. Thank you for report

ntfs1984 commented 6 months ago

Added callback_object to public field of class to make it visible outside. Not sure how to explain this correctly.

GObject.h:

        public:
struct st_callback;
st_callback *callback_object;

Then added deletion of this var into handler_disconnect function.

GObject.cpp:

void GObject_::handler_disconnect(Php::Parameters &parameters)
{
    Php::Value callback_handle = parameters[0];
    delete callback_object;
    g_signal_handler_disconnect(instance, (int)callback_handle);
}

So now we can run handler_disconnect when necessary and it will also release memory used by struct. Tested, this works, so you can get this idea.

scorninpc commented 5 months ago

Ok, i got, but if you disconnect before reconnect your self with PHP, without delete, still leaking memory?

Because what are telling make sense, but all structures on callback_object are primitive like or small classes. it's free some bytes only

I'll update with this code, because make sense free the object, but it's not will leak so many memory

Anyway, you cannot do this object as public, beause all widget will share the same object. You may need to register allocated memory with the same signal id, and free it after by this ID

I'll try to do something today and return here

ntfs1984 commented 5 months ago

Yes, it's not leaking so many memory, just about 1Kb for iteration. But if we have many iterations - this leak is sensitive.

If we'll refresh every 250ms - our program will eat + 1Mb every 4.5 min, or + 1Gb every 3 days. It's so much. You can check with my initial example.

scorninpc commented 4 months ago

Store a list of structures of each signal of widget may be a pain, so I found a simplest way https://github.com/scorninpc/php-gtk3/commit/d176b1472bbb2757cf56fbe383bb808cbcf06369

image

Review please and close if it's OK

ps: sorry for delay, hard work days

apss-pohl commented 4 months ago

I compiled for windows, now i get a lot of those:

(php.exe :4948): GLib-GObject-CRITICAL **: 09:24:38.068: g_object_weak_unref: couldn't find weak ref 00007ffbe68f7e90(0000026b7388c0f0)
scorninpc commented 4 months ago

Can you provide Gtk, GLib and Pango version?

https://bugzilla.mozilla.org/show_bug.cgi?id=503792 https://gitlab.gnome.org/GNOME/loupe/-/issues/100

After save versions, try to update your libs

apss-pohl commented 4 months ago

mingw-w64-x86_64-gtk3 3.24.39-1 mingw-w64-x86_64-glib2 2.78.3-1 mingw-w64-x86_64-pango 1.50.14-4

Versions are stable, once i revert the changes the error disappears using same environment.

scorninpc commented 4 months ago

Yes, the reverted version does not have g_object_weak_unref. All i found on web tell this is a glib related

image

Can you upgrade glib to 2.80? yours is 2.78

apss-pohl commented 4 months ago

I just noticed that it is even more complicated.

We are stuck on php7.4 and there is an even more minor version in place: glib-2.53.3-vc15-x64.zip This is part of the official php build procedure which is also in place on the Tutorial we have on this repo. Updating can lead to unforeseen effort and issues. Even php 8 for windows will use outdated version: glib-2.53.3-1-vs16-x64.zip

scorninpc commented 4 months ago

for real? i have 8.1 compiled here!

I'll do another VM to make a new compile env, and try to do some tests

But i'm sure that problem is part of GTK/Glib/Pando problem, not PHP-GTK it's self

Maybe try clear structure by another way to avoid this bug, but ... need tests

scorninpc commented 4 months ago

Did you try update only on mingw?

image

apss-pohl commented 4 months ago

Yes i tried this and the version is "mingw-w64-x86_64-glib2-2.80.0-1"

I also updated the build depedencies for php itself against the lastest versions from msys and rebuild it +gtk3 on top of it. But the issue persists..

PS. Also happen on linux

scorninpc commented 4 months ago

sad to hear that! i think i'll try to avoid g_object_weak_unref method

Alot issue about that https://github.com/search?q=g_object_weak_unref&type=issues https://github.com/search?q=%22couldn+t+find+weak+ref%22&type=issues

can you try to run with param GSETTINGS_BACKEND=memory please?

GSETTINGS_BACKEND=memory php.exe test.php for example

edited: merge posts

apss-pohl commented 4 months ago

Just tried it but i don't see anything changed. What to expect? IF you are online we can also chat on discord for testing

ntfs1984 commented 4 months ago

Just tested. Works fine without any tricks, thanks.

@apss-pohl - tested with php8.1 on archlinux - running without problems.

apss-pohl commented 1 month ago

Hi Bruno,

do you consider to revert that change? g_object_weak_unref still causes the issues

scorninpc commented 1 month ago

Hey, how are you!

Yes, i'm waiting you confirm if I can revert

apss-pohl commented 1 month ago

Well i am using it without the "g_object_remove_weak_pointer" all the time very intensive, i don't have any issues. Here is a PR: https://github.com/scorninpc/php-gtk3/pull/132

scorninpc commented 1 month ago

Ah ok! Very nice

But i think you can remove this PR, because there is more thing to remove https://github.com/scorninpc/php-gtk3/commit/d176b1472bbb2757cf56fbe383bb808cbcf06369 and revert is more easy

apss-pohl commented 1 month ago

Whatever fits best for you. I only did this one liner and it seems fine, but i also have no clue what the other stuff will do

scorninpc commented 1 month ago

fixed https://github.com/scorninpc/php-gtk3/commit/09e9cdec470c1fef8682133baa013c447cc8e753

i just comment methods, becase i loved to find that to just remove it now hahah