New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Widgets no longer being freed after 3.5.1 #211

Closed
gnunn1 opened this Issue Aug 4, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@gnunn1
Contributor

gnunn1 commented Aug 4, 2017

In Tilix I've added some code ages ago to various high level widget destructors that outputs whether the destructor is called or not. I should be paying more attention to this on GtkD version updates, however I was looking into memory usage recently and noticed a change in behavior starting with 3.6.0.

In 3.5.1 if I close a tiled terminal then execute a GC collection a couple of times I can see the various destructors (VTE, Terminal) being called as expected. However starting in 3.6.0 this no longer happens and the destructors are not called until the program terminates. Now I know that in D destructors are not guarenteed to be called but I use this as a rough barometer with regards to whether I'm leaking objects.

Looking at the changes made between the two versions, there is one thing that sticks out to me. In commit 4b491f5, the event listeners array were changed from being class members to static members of the wrapper. I was wondering if this could be causing a reference to the delegate being held onto to since it never gets removed from the static unless explicitly removed.

MikeWey added a commit to gtkd-developers/gir-to-d that referenced this issue Aug 4, 2017

Fix memory leak with the signals.
Because the array was static the delegates and what there this pointer was pointing to were never freed.

See Also: gtkd-developers/GtkD#211

MikeWey added a commit that referenced this issue Aug 4, 2017

Fix a memory leek with signals.
With the static array the classes are never freed.

See #211
@MikeWey

This comment has been minimized.

Member

MikeWey commented Aug 4, 2017

I think that is indeed the problem.

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Aug 5, 2017

I see you made some commits, not sure if it's ready for testing but I gave it a shot by building tilix against GtkD master. Unfortunately, no change as I still see the same behavior from the 3.6.x series though I can see the static listener array has been removed.

I'll do some more digging on my end, if you want to try tilix yourself to see the issue I can post the steps I'm using to build and reproduce it.

@MikeWey

This comment has been minimized.

Member

MikeWey commented Aug 5, 2017

I'll do some more digging on my end, if you want to try tilix yourself to see the issue I can post the steps I'm using to build and reproduce it.

Yes, could you post the steps to reproduce the issue.

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Aug 5, 2017

  • Update dub.json and dub.selections.json to 3.5.1

  • Compile tilix using dub build --build=debug --config=trace

  • If you don't have tilix installed, you will need to do via sudo ./install.sh

  • Run tilix from another terminal, you should see a bunch of trace logged to stdout

  • In Tilix split a terminal using one of the buttons in the header bar

  • Close the new terminal by pressing it's close button

  • In the hamburger menu in the header bar click the GC option. You should see something like *** VTE Destructor

  • Click GC again and you should see *** Terminal destructor

  • Close tilix and compile it with 3.6.5. Repeat the steps above, no destructors are logged until you close tilix.

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Aug 5, 2017

Did all have the listeners get re-generated properly in master after that last commit, I'm still seeing some declared as static:

grep -R "static.*listeners" *
gtkd/gobject/ObjectG.d:		static OnNotifyDelegateWrapper[] listeners;
gtkd/gtk/Widget.d:			static ScopedOnDrawDelegateWrapper[] listeners;
gtkd/gtk/Widget.d:			static OnDrawDelegateWrapper[] listeners;
gtkd/gtk/ComboBoxText.d:		static OnChangedDelegateWrapper[] listeners;
gtkd/gtk/ComboBoxText.d:		static OnFormatEntryTextDelegateWrapper[] listeners;
gtkd/gtk/ComboBoxText.d:		static OnMoveActiveDelegateWrapper[] listeners;
gtkd/gtk/ComboBoxText.d:		static OnPopdownDelegateWrapper[] listeners;
gtkd/gtk/ComboBoxText.d:		static OnPopupDelegateWrapper[] listeners;
gtkd/gio/Application.d:		static ScopedOnCommandLineDelegateWrapper[] listeners;
@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Aug 5, 2017

My memory is returning, those are the hand written ones if I remember correctly. I'll comment out the places I'm using those and test again.

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Aug 5, 2017

Once I comment out my onDraw handlers everything works as expected so the change of listeners to static is definitely the issue.

MikeWey added a commit to gtkd-developers/gir-to-d that referenced this issue Aug 6, 2017

Fix memory leak with the signals.
Because the array was static the delegates and what there this pointer was pointing to were never freed.

See Also: gtkd-developers/GtkD#211

MikeWey added a commit that referenced this issue Aug 6, 2017

Fix a memory leek with signals.
With the static array the classes are never freed.

Fixes #211
@MikeWey

This comment has been minimized.

Member

MikeWey commented Aug 6, 2017

I've fixed the manually added signals, and tagged version 3.6.6.

@MikeWey MikeWey closed this Aug 6, 2017

@gnunn1

This comment has been minimized.

Contributor

gnunn1 commented Aug 6, 2017

Thanks @MikeWey, works great.

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