Update metadata in the treeview without reload all - Fixes #4852

[godiard]

Implement a set_value method in the model, and move the logic
to write to the datastore to the ListModel. To be able to
update the model without trigger a update in the treeview,
we pass the updated callback in the ListModel.setup() method,
and the callback is disconnected and connected in every update.

The treeview have the responsabilty of redraw the cell after
the change was done.

The listmodel stores a dictionary with the entries uodated,
to provide that data until the next update in the Journal.

This patch is heavily based in the work of
Sam Parkinson sam.parkinson3@gmail.com

[tchx84]

Just wondering, but would it make it more complex to also cover "details view" metadata?

ie., if you scroll to some far below entry and go to details view, any change done to the entry metadata will make it reset the view when pressing the "back" button.

[tchx84]

Another case I noticed:

1. star some entries, say four..
2. filter entries by pressing the star in the journal toolbar.
3. un-star one of the entries, and the entry does not disappear (which is a different behavior from previous).

[godiard]

@tchx84,
Replying to your first comment: yes, will be much complicate.
Second comment: is true. I don't know if the change is bad for the user (can revert more easily a change) but is a change.

[samdroid-apps]

@godiard

1. Couldn't we just emit a signal from the expanded view's _write_entry method and add another patch to the list view based on that?
2. I agree with you.

[tchx84]

@godiard, @samdroid-apps lets double check (by code analysis and test) that this doesn't break something else and that it does not change another more-relevant behavior.

[samdroid-apps]

@tchx84 Should we merge this early in this cycle to get the most testing happening (as @godiard suggested in the meeting)?

[tchx84]

@samdroid-apps @godiard , a couple questions to make sure I understand it properly:

1. Do we only store a copy of the entries metadata that have been changed by the user?
2. When does these copies get freed from memory?

[godiard]

@tchx84

1. Do we only store a copy of the entries metadata that have been changed by the user?
   yes. We have a copy of the metadata of modified entries only.
2. When does these copies get freed from memory?
   According to my tests, the ListModel will be initiated again the next time the Journal is displayed, if you change to other activity or view, or if the Journal is updated from the UI. (You can add a log to ListModel.init to found that).
   We need confirm if that is still valid when Change model query in place for Journal ListView, fixes #4852 #542 is applied. Then even if this and Change model query in place for Journal ListView, fixes #4852 #542 are valid improvements, we need check the interaction of both.
   I didn't tested if Change model query in place for Journal ListView, fixes #4852 #542 solves the issue we try to solve here.

[samdroid-apps]

Replaced with #564

[samdroid-apps]

Hum, this patch appears to leak memory. Here is a graph of jarabe's memory consumption when the patch is applied:

The horizontal axis is time and the vertical is virtual memory.

What I did while making this graph:

1. Open the journal
2. Toggle the star of every item
3. Rename every item LOL
4. Click the documents view (this should flush the patchset)
5. Wait a while

[samdroid-apps]

Ok, so here are 2 more tests about this patch:

1st, a test where del self._updated_entries was added to the stop function of the listmodel (using the same method, but 111 rather than LOL):

2nd a test on the master branch (I only toggled the stars of the top entries, and I didn't change the labels of any - the scrolling bug is very annoying!):

So, it appears that both before and after the patch, the journal eats ram.

I am willing to merge this patch as. It works very well, it is fast and it fixes the bug. It does not have any side effects that I have found through testing.

[quozl]

This might not be a leak. Virtual memory may be released by Python yet be moved to a free list for reallocation. To prove it, I suggest automating and repeating the test sequence you used ... and have it run at least ten times while recording virtual memory size of shell and datastore processes.

[godiard]

Interesting test. What method did you use to graph the memory consumption?

[samdroid-apps]

@quozl I agree with you that python is probably not releasing the memory. I'm pretty sure there are a few bugs about how python releases/doesn't-release memory that got fixed in newer versions, maybe only in 3.x though.

@godiard I measured the memory of the jarabe process. My method was:

1. Start sugar
2. Use htop to find jarabe pid
3. In fish shell, run:

set pid 11111
while true
    echo (date "+%s%N") (cat /proc/$pid/status | grep VmSize | grep -o "[0-9]*") >> data
end

1. Stop the loop after completing the said actions
2. graph -T png data > graph.png

[quozl]

Cool, it has been about 20 years since I last used plotutils, with an actual plotter. ;-)

For comparison, here is a method that I used; you might find it looks nicer and is easier to customise yet still scriptable;

while true; do
  echo $(date +%s.%N) \
    $(awk '/VmSize/ { print $2; }' /proc/$(pgrep -f jarabe)/status)
  sleep 1
done | tee memory.log

Plotting done with gnuplot package with this shell script; comment out the set terminal and set output lines to display on screen;

#!/bin/bash
gnuplot -persist <<EOF
set terminal png
set output "memory.png"

set title "XO-1.5 ... \nVmSize leak test\n2015-10-06\nby James Cameron"
set xlabel "Clock time MM:SS"
set ylabel "VmSize kb"
set nokey

set xdata time
set timefmt "%s"

set grid
set style data step

plot "memory.log" using 1:2 title "run #1" lw 2
EOF

Result looks like this ...

[samdroid-apps]

That's so cool! Plus you found a bug in the background cpanel :)

Did you find this journal patch to cause memory leaks?

[quozl]

No, I haven't got an automated test case for that yet. I did check recently on #4852 to make sure the problem was still happening.

[samdroid-apps]

Test case: toggle star of 10 journal entries, switch to document volume, switch back to journal volume x10

Test: toggle star on 10 jobjects, switch to running browse activity (via frame), switch back to journal

[quozl]

Thanks. Ach, crivens, I smell a rat! 🐀

I did a quick test of several normal UI transitions in Sugar 0.106 while monitoring VmSize of Jarabe. Every single transition tested leaks some amount of memory, which over time will accumulate unless there's something to clean it up. It is critical on the XO-1, which only has 256 MB of RAM.

Leak rate, KB/sec	Transition
15	home view list and ring switching
18	home view and journal switching (f3/f4)
21	frame show and hide
57	network and buddies switching (f1/f2)
85	network and home view switching (f1/f3)

It will be interesting to see what causes this. As you see the same leak rate with and without your patch, my guess is that we have a wider problem. No need to hold up your patch.

My log analysis script:

#!/usr/bin/python
import glob

for name in glob.glob('memory*.log'):
    lines = file(name, 'r').readlines()
    t0 = float(lines[0].split()[0])
    t1 = float(lines[-1].split()[0])
    v0 = float(lines[0].split()[1])
    v1 = float(lines[-1].split()[1])
    print '%.0f | %s' % (((v1 - v0) / (t1 - t0)), name)

[quozl]

The leak rate seems proportional to the size of icons and images processed. An automated test method using xdotool has been added to bug #4889, for your interest.

[godiard]

If the leak rate is proportional to the icon sizes, the obvious place to look is icon.py.
I was looking at a few different places, but without find anything significant.

• There are two LRU caches, one for the _SVGLoader, and the other in the _IconBuffer. I added log in the method get_surface() to check if the surface was found in self._surface_cache, and many surfaces where found, what that suggest the cache is working.
• In the LRU (sugar3/util.py) the delitem is called. Maybe we need clean something more to notify to the Gtk bindings to clean the memory? To try confirm this, I have tried not use the _surface_cache at all, and the leak continued.
• In one of my test I found that just going to the Journal and moving the mouse over the Journal without click or change view for a while increase the memory. I added a return at the beginning of the do_render() method in the CellRendererIcon class (the the icons are not displayed) but the memory increase anyway....
• The _IconBuffer class has a self.cache property by default on False. I didn't find any place where that value is changed. This property is used to say to the SVGLoader to cache or not the svg data.
• I don't know if can be related, but the CairoSurface have a method finish() http://cairographics.org/documentation/pycairo/2/reference/surfaces.html#cairo.Surface.finish

[samdroid-apps]

I think that the status of memory leaks in the journal should be an important discussion, but is not related to this patch. There are memory leaks before and after this patch?

@godiard git show 64ceb3e30f97a14f5c698af43a59c15c8f70e050 says I did the commit. Do you want to fix the author?

[quozl]

@samdroid-apps, it's not the journal, it's everything. 🐀

[samdroid-apps]

Merged!

[samdroid-apps]

Oh no, I've done something bad with my merge

[samdroid-apps]

Yep this merge when really badly and github has seemingly stripped gonzalo's name from the patch.

@godiard please re submit the patch with your authorship!

I will revert the merge my apologies to everybody. I will merge the patch when it is corrected as the discourse has moved beyond this patch already.

[godiard]

@samdroid-apps, no problem for me, you did all the heavy work.