Sugar shell consuming vast amounts of memory

[kimquirk]

Build 653

Testing for this release showed that after 1-2 days activities refused to load.

This has been traced to a memory leak in the sugar shell. May need to come up with a short term solution; and a better longer term solution.

[marco]

Tomeu, I checked out the (hippo/gtk) destroy thing. It works for the case where you are connecting to the widget/item which is being destroyed (usually self) but obviously it doesn't work if you are connecting to a widget/item your class owns (the model case).

[marco]

Break the reference cycle using the destroy signal

[marco]

I attached a testcase for another possibility we might consider. It's not great, but at least it keeps the hacks inside the icon implementation.

[marco]

The medium term solution for BuddyIcon is probably to rework the way we interface with the PS.

[tomeu]

We were leaking a set of objects that were instantiated at every BuddyAppeared signal. That includes dbus proxies, and model and UI objects.

We were leaking because of three causes:

• Palette was hold by a reference in PaletteGroup and by a bound method referred in PaletteObserver.
• sugar.presence.buddy.Buddy was hold by the dbus signals it listens to and by the object cache in PresenceService.
• BuddyMenu was hold by a signal in HomeModel it listens to.

The attached patch fixes all three and now we only grow a dbus.ObjectPath per buddy. This is very small compared to the previous situation, but I still want to know if it's a leak or just a cache inside dbus-python.

Marco, can you give a look to the attached patch? Perhaps there are simpler solutions to those.

[tomeu]

script for creating fake buddies in the mesh view

[tomeu]

hacks in sugar-shell to try to find the leaks

[marco]

Tomeu, when you have something which can be reviewed please let me know. I think we should try to get something in before the freeze, even if small leaks are left.

[tomeu]

proposed partial fix

[tomeu]

The patch attached reduces greatly the amount of memory leaked when buddies appear and disappear.

We still leak big chunks of memory when activities appear and disappear from the mesh view. I'm trying to hunt that one down.

I hope I can find that one soon, if not, I think applying this patch will be enough for update.1.

[marco]

With this patch applied we still rely on the cyclic gc to release the objects, correct?

[marco]

Both gtk widgets and hippo items already support explicit destruction. I think we can reuse it and cleanup code a bit. I can come up with a patch for that.

We should document how signals should be connected/disconnected because that's probably the more common cause of leaks.

I suggest we split the patch in two (UI and PS) so that they can be cleaned up and checked in separately.

[tomeu]

The last three patches fix the worst leaks, those caused by buddies appearing and disappearing.

Each patch can be applied separately and fixes separate reference leaks, but all of them need to be applied so objects in the cycle are correctly collected.

[marco]

review+ for fix_leaks_lib.patch

[marco]

Review for fix_leaks_ps.patch:

+        self._s1 = None

I think we should merge _add_items in the constructor. Please also get rid of the commented out code while you are at it.

+            shell_model = self._shell.get_model()

We are doing this in so many places... I think we should store the model in an instance variable.

+        self._s1 = home_model.connect('active-activity-changed',

I think we should use descriptive names even if gets more verbose. I'm generally using hid as a suffix to indicate it's a handler id.

         self._layout.remove(icon)
         del self._activities[activity_model.get_id()]
+        icon.destroy()

I think destroyed items are removed automatically, so the remove would be unnecessary, can you verify please?

+    def __destroy_cb(self, activity_icon):
+        for key, icon in self._icons.iteritems():
+            icon.destroy()
+
+        self._icon.destroy()

I don't understand why this is necessary. Killing an item should also explicitly destroy all of its children automatically. If you just self._icons = None do we still leak?

(Please make sure to pylint all the patches, /me trying to get used to always do it)

[marco]

We should have collabora review fix_leaks_ps.patch

[tomeu]

Replying to marco:

Review for fix_leaks_ps.patch: {{{ + self._s1 = None }}} I think we should merge _add_items in the constructor. Please also get rid of the commented out code while you are at it.

That code is not mine, but I think that having a separate _add_friends() method makes the code more readable. The constructor would be quite big and we wouldn't have a way to tag that block of code if we don't add a comment.

{{{ + shell_model = self._shell.get_model() }}} We are doing this in so many places... I think we should store the model in an instance variable.

If the problem you see is duplicated code, then I would prefer to add the auxiliary methods _get_shell_model() and _get_home_model() to BuddyMenu instead of adding more references. In my opinion, those classes should be singletons and accessible from any class in the shell code.

{{{ + self._s1 = home_model.connect('active-activity-changed', }}} I think we should use descriptive names even if gets more verbose. I'm generally using hid as a suffix to indicate it's a handler id.

Ok

{{{ self._layout.remove(icon) del self._activities[activity_model.get_id()] + icon.destroy() }}} I think destroyed items are removed automatically, so the remove would be unnecessary, can you verify please?

If we don't do self._layout.remove(icon), we still leak. By reading the code, looks like CanvasBox listens for 'destroy' in its children, but SpreadLayout doesn't, so this is needed for the moment.

{{{ + def destroy_cb(self, activity_icon): + for key, icon in self._icons.iteritems(): + icon.destroy() + + self._icon.destroy() }}} I don't understand why this is necessary. Killing an item should also explicitly destroy all of its children automatically. If you just self._icons = None do we still leak?

Sorry, this was a left over from a try to eliminate the leak when activities come and go. I haven't managed yet to fix this leak.

(Please make sure to pylint all the patches, /me trying to get used to always do it)

Any idea how to reduce the false positives? Looks like pylint cannot resolve the inheritance from sugar classes to GObjects.

pylint.sh in sugar always tells me my code is perfect.

[marco]

Replying to tomeu:

{{{ + shell_model = self._shell.get_model() }}} We are doing this in so many places... I think we should store the model in an instance variable.

If the problem you see is duplicated code, then I would prefer to add the auxiliary methods _get_shell_model() and _get_home_model() to BuddyMenu instead of adding more references. In my opinion, those classes should be singletons and accessible from any class in the shell code.

Sounds like a good idea to me.

{{{ self._layout.remove(icon) del self._activities[activity_model.get_id()] + icon.destroy() }}} I think destroyed items are removed automatically, so the remove would be unnecessary, can you verify please?

If we don't do self._layout.remove(icon), we still leak. By reading the code, looks like CanvasBox listens for 'destroy' in its children, but SpreadLayout doesn't, so this is needed for the moment.

Hmmm we will have to look into it at some point, but for now it's fine.

Any idea how to reduce the false positives? Looks like pylint cannot resolve the inheritance from sugar classes to GObjects.

afaik the only way is to disable those errors in the pylint config. Obviously that's going to hide some real errors though :(

[tomeu]

<daf> tomeu: patch looks ok
<daf> tomeu: though you could give _s1 a better name :)

[tomeu]

pushed to git

[tomeu]

Marco, can you include it in a rpm? Thanks!

[marco]

I don't see regressions but this obviously will require more testing. Reassigning to u.2 to keep looking into shell memory usage.

We should also come up with guidelines on how to avoid these sort of issues (both for activities and the shell).

[mstone]

Leaks, if they still exist, are slow enough to be undetectable in casual tests of update.1-70x builds (e.g. by letting the build run overnight at 1cc).