#5532 HIGH Update.: Sugar shell consuming vast amounts of memory

[Zarro Boogs per Child]

#5532: Sugar shell consuming vast amounts of memory
-----------------------+----------------------------------------------------
  Reporter:  kimquirk  |       Owner:  tomeu   
      Type:  defect    |      Status:  new     
  Priority:  high      |   Milestone:  Update.1
 Component:  sugar     |     Version:          
Resolution:            |    Keywords:          
  Verified:  0         |    Blocking:          
 Blockedby:            |  
-----------------------+----------------------------------------------------

Comment(by tomeu):

 Replying to [comment:14 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.

-- 
Ticket URL: <http://dev.laptop.org/ticket/5532#comment:16>
One Laptop Per Child <http://dev.laptop.org>
OLPC bug tracking system