Optim/no refresh on hidden pane

[jaesivsm]

Depends on getting-things-gnome/liblarch#22 won't self.refilter on non-displayed tree, cutting by 3 the display time when selecting a tag.

Context

When selecting a tag with around 150 tasks, the process time gets up to 1s5. Mainly I found that a lot of callbacks are called in liblarch but as I don't fully understand that library, I didn't do much on that side.

I did witness though, that for all three panes the apply_filter function, each times, calls on refilter. That last one browse the tree in lots of ways.
Although, and that's where I have doubts, all three refilter aren't necessary, and it's only needed for the displayed pane.
The fix is to only trigger the refresh for the active pane.

[nekohayo]

Isn't this docstring a lie now? (and the one in unapply_filter_on_panes too)

[jaesivsm]

Well yeah, I'll adjust the docstring, but indeed, by default with the force_refresh=False all pane won't be applied filters upon.

[nekohayo]

Given how important this seems to be to maintain somewhat decent performance, I think it might be good to add some comment(s) in the code to explain the gotchas / warn people to only refresh the currently shown pane to cut down the performance impact massively, avoiding potential future naïve code mistakes?

[nekohayo]

Wow so... if I understand correctly, you're saying the app was asking liblarch to refresh "All tasks" + "Actionable" + "Closed" everytime you click a tag in the sidebar? Interesting finding, that explains why toggling the Workview in 0.3.x was slow but it isn't in 0.4+;

Silly question though: what happens now when you switch between panes, it will have to refilter the view, which means switching between panes will no longer be instantaneous? We might be bringing back an old problem I guess, unless there's a way for the filtering to happen first in the current view, and then "asynchronously, in the background" do it for the 2 other views as well...

[Neui]

Wow so... if I understand correctly, you're saying the app was asking liblarch to refresh "All tasks" + "Actionable" + "Closed" everytime you click a tag in the sidebar?

From what I've seen while trying to check the performance, yes. I thought that was intentional ("switching tabs is fast now"), and also thought of doing something similar to this PR (but with your async suggestion), but I ultimately wanted to rather fix the "underlying" issue first (current unsuccessfully).

[jaesivsm]

Wow so... if I understand correctly, you're saying the app was asking liblarch to refresh "All tasks" + "Actionable" + "Closed" everytime you click a tag in the sidebar?

Yup.

I thought that was intentional

Same here, hence the PR and the open question. Although, in the call to liblarch, the refresh parameter was actually ignored. So i wasn't sure about it entirely.

Silly question though: what happens now when you switch between panes, it will have to refilter the view, which means switching between panes will no longer be instantaneous?

I didn't see issues while testing around but that's mainly what I was worried about indeed.
However if some more refiltering is needed I guess it would be more appropriate for it to be only "just on time" for the current panel. So I might need adding a call to apply_filter_on_panes when switching pane...

[nekohayo]

I tried this out with your liblarch branch, and I saw a 2x improvement with various operations when running the "bryce" sample data set (./launch.sh -s "bryce"), which has 827 tasks. However, I encountered a bug: it breaks the Actionable view on that data set.

Git master

• startup time: 23.0 seconds
• switching to "Actionable" view: 0 secs
• switching back to "Open" tasks view: 0 secs
• switching to "work" tag: 6.2 secs
• switching back to all tasks: 13.6 secs

With your optimizations branch

• startup time: 12.7 secs
• switching to "Actionable" view: BROKEN, no tasks are displayed at all!
• switching back to "Open" tasks view: 0 secs
• switching to "work" tag: 3.6 secs
• switching back to all tasks: 6.6 secs

Note: performance gains on the "bryce" dataset are 2x rather than the potential 3x, because that dataset doesn't have more than 1 closed tasks in it, so from a data perspective it's almost as if it only had two panes in practice.

[jaesivsm]

Ok so this time it should be good
Also I've redone the whole logic. (thanks again to the -s bryce dataset !)

I've dropped the previous logic of unapplying one by one every filter. We now reset the whole bunch since there is no performance gain in not doing so (no refilter() is called between those unapplication). Plus, unapplying implied keeping a separate list of selected tags that comes as a duplicate of the filters list that liblarch is keeping.
We now reset the filter list and trigger refilter() if needed on every tag / pane switch but only for the current pane.

[Neui]

Looks good to me, but I want to test it for myself.

[Neui]

No exception details? (e in the old code)

[jaesivsm]

Yeah, got dropped in the heat of the rewrite, i'll add it back again.

[Neui]

Why awkwardly filter_ and not just filter (no _)?
I also don't like the is_last thingy, but I guess you used it to let liblarch decide whenever it really should refilter (https://github.com/getting-things-gnome/liblarch/blob/8c90053b9a3593706cdf082353b4146ff79a8f55/liblarch/filteredtree.py#L611-L631). Maybe add a comment and/or something in the commit message about this

[jaesivsm]

Well, filter is a python reserved keyword, and it's not recommended as a rule to override reserved names, as for filter_ especially, it's the PEP8 recommended behavior.. But I can find a synonym.

I'm not a big fan of the is_last thingy either, and I'd rather prefer explicitly call for refilter(). Better yet, a new liblarch method allowing to apply multiple filters at once. But since I didn't want to alter too much of the liblarch API and still wanted to limit calls I thought of this. But yeat, a comment is indeed needed.

[Neui]

Well, filter is a python reserved keyword, and it's not recommended as a rule to override reserved names, as for filter_ especially, it's the PEP8 recommended behavior.. But I can find a synonym.

But filter is a built-in function (or class as the python3 interpreter says), not a reserved keyword from what I can see. I can redefine it at least in the python REPL, so I'm sure it isn't a keyword.

[jaesivsm]

Yes indeed, keyword and not built-in function, sorry about the mix up.
But to me it's still overriding a declared value. From experience it may lead to some complicated debugging. So yeah I usually follow the PEP8 recommendation about keywords on that too.
But since it's important I'll use the name from the ViewTree.apply_filter function.

[Neui]

Not that important but somewhat annoys me:
reseting → resetting
applieds → the applied
wether → whether

[jaesivsm]

Comments are important :) Fixed !

[Neui]

Doesn't really say why you couldn't just do refilter. Maybe append "This way, liblarch can prevent refreshing in certain situations rather than refilter all the time. (also see resetting above)"
or "... last item, possibly preventing a refilter (see above when resetting)"

[jaesivsm]

Ok I'll edit that. But truth is, I can't call on ViewTree.refilter() because there is no such method. ViewTree doesn't proxy that method to FilteredTree and I don't know why at all. This is what I meant in the second part of that comment.

[Neui]

Looks good to me, switching seems to work as intended. However, it takes longer to start, around 50s instead of 37s on current master (when the "Open" view is selected), and seems to be rather consistent. On top of longer startup time, it'll refresh when switching views/tabs directly after startup, so it looks like it force refreshes or something.
(Side note: On a new day, which refresh_all_views is called, took 3:10m, but it doesn't use any of the modified methods, so maybe don't really worry for this PR; Haven't tested how long it would be when on master.)

[jaesivsm]

Hum I'm not sure I get how you obtain those figures. Is there a defined way to measure these ? Especially the startup time, all I could come up with was to measure the time for do_startup and do_activate method execution.
I had to write a decorator to count time passed in a function and here's what I got :
(everything is executed right after a git clean -fdx, because I have no idea of the caching that actually takes place and with the -s bryce dataset)

Note : for the patch to actually improve perf you have to have liblarch on the branch mentioned in the first message of this PR. If not, the refresh won't be avoided and it might indeed take more time.

On master branch :

Startup gives up :

| do_startup: 1594.282ms
| | init_style: 0.776ms

| do_activate: 9532.568ms
| | init_browser: 9484.415ms
| | | __init__: 9484.368ms
| | | | on_select_tag: 6108.745ms
| | init_actions: 1.070ms
| | init_plugin_engine: 0.090ms

Selecting the ubuntu tag :

| on_select_tag: 2525.094ms

With the patch :

Startup :

| do_startup: 1621.952ms
| | init_style: 0.757ms

| do_activate: 6719.164ms
| | init_browser: 6679.087ms
| | | __init__: 6679.042ms
| | | | on_select_tag: 3412.005ms
| | init_actions: 0.934ms
| | init_plugin_engine: 0.077ms

Selecting the ubuntu tag :

| on_select_tag: 771.174ms

Of course, and that's the matter of the patch, selecting a different pane takes time the first time.
Here, with ubuntu tag selected, swithing to the "closed" pane:

| on_pane_switch: 1187.721ms

[jaesivsm]

After the previous comment from @Neui I got curious about startup time (wasn't really the subject of the PR at first), and I saw that liblarch.FilteredTree.refilter() was called during window initialization a bunch of time.
I could scrap one of those :

@@ -409,8 +406,6 @@ class MainWindow(Gtk.ApplicationWindow):
         self.vtree_panes['closed'].connect('key-press-event', clsd_tsk_key_prs)
         self.vtree_panes['closed'].connect('cursor-changed', self.on_cursor_changed)
 
-        self.closedtree.apply_filter(self.get_selected_tags()[0], refresh=True)
-
         b_signals = BackendSignals()
         b_signals.connect(b_signals.BACKEND_FAILED, self.on_backend_failed)
         b_signals.connect(b_signals.BACKEND_STATE_TOGGLED, self.remove_backend_infobar)

salvaging another second on my test (9s on master, 6s previously, roughly 5s here). And since the method on_select_tag (now _reapply_filter) was called afterwards the call was useless.
I also noticed that during do_startup the registering backend phase, was actually triggering a lot of liblarch.FilteredTree.__update_node call (without getting through a refilter()) which adds up to some incompressible 1s5.
That might be another optim for another PR right there.

[Neui]

Hum I'm not sure I get how you obtain those figures. Is there a defined way to measure these ? Especially the startup time, all I could come up with was to measure the time for do_startup and do_activate method execution.

Sorry for not being more specific. In this case I just ran ./launch.sh with my own dataset (not bryce, and I won't provide it, sorry), and started counting with the help of the clock after it install/seeing a warning, until the main window appeared.

Note : for the patch to actually improve perf you have to have liblarch on the branch mentioned in the first message of this PR. If not, the refresh won't be avoided and it might indeed take more time.

Makes sense, I forgot to apply it. I'll try it with that patch later/this evening (GMT).

[A bunch of exlpaination...] That might be another optim for another PR right there.

I'd be ok to put that into another PR.

[Neui]

Note : for the patch to actually improve perf you have to have liblarch on the branch mentioned in the first message of this PR. If not, the refresh won't be avoided and it might indeed take more time.

Makes sense, I forgot to apply it. I'll try it with that patch later/this evening (GMT).

Well I missed it, so I did it now.
With the liblarch PR applied, it takes around 27s for the main window to appear on master, and around 12s with this PR applied.

GTG refreshes when waking up/new day occurs. I took an bit different route and enabled the developer console (in the extensions), opened it (from the menu top right), app.timer.emit_refresh(), wait until you get a return value. It took around 2m30s on master, and 5s on this PR. It also "invalidates" the other tabs (so it needs to wait when changing views), but I didn't test whenever it correctly changes the dates.

One thing I noticed was that when switching to the "closed" tab (that is big since I disabled task cleanup), it takes around 18s (from clicking until responding/changing the view visually). Since master already "filters" the tab, it seems a bit odd to me that this PR takes a bit longer (12s startup + 18s switch = 30s). Not too bad though.

I'll review the new code changes soon.

[Neui]

Looks good to me and works.

[nekohayo]

I've tested this again and the issue I had seen before seems gone. Performance is a net improvement (see my updated benchmark results in #402 (comment) if you're curious), I'm excited and looking forward to further optimizations you may come up with! So I will now merge this as-is, and hope @diegogangl won't encounter too many (if any) conflicts with his big WIP branches ;)