469691 - Unnecessary refresh of views

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Ludovic Marcotte]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4
Build Identifier: 0.9

Lightning does way too many refresh on views and that slows down the app considerably.

For example, if I have 7 calendars (A...G), 6 of them are displayed and I decided to display the 7th one, a refresh call will be done (as onCalendarAdded will be posted) and getItems() will be invoked on the composite calendar, for all views. This will generate 7 getItems() calls per view.

It's actually worse when you have zero calendar displayed, and you decide to show them all (or the opposite, all of them are display and you decide do 'hide' them all). onCalendarAdded() will be posted for each added calendar, one at the time - it'll result in tons of getItems() calls due to the composite calendar. In the example above, you'll end up having the following calls for getItems:

A
A,B
A,B,C
A,B,C,D
A,B,C,D,E
A,B,C,D,E,F
A,B,C,D,E,F,G

So, n*(n+1)/2 database calls, _per view_.

Lightning has the agenda list box, minimonth view, task view, month view and multiday view. For those, it would generate at least 140 getItems() calls... followed by relayouting!

The idea behind the patch attached to this bug report is to:

1- not refresh an undisplayed view
2- not refresh when doing batch operations

It also contains a patch for https://bugzilla.mozilla.org/show_bug.cgi?id=469605.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Ludovic Marcotte]

Attachment: Initial version

Comment 2

[Bas van den Bosch]

drawing the views makes Lightning slow was already noted in i.e. bug 420615. Wanted 1.0 and Blocking 1.0?. Also, this is not just a mac-issue but general platforms.

Comment 3

[Simon Paquet [:sipaq]]

If this gets confirmed, this is clearly a 1.0 blocker.

Comment 4

[Daniel Boelzle [:dbo]]

(In reply to comment #0)
> Lightning does way too many refresh on views and that slows down the app
> considerably.
> 
> For example, if I have 7 calendars (A...G), 6 of them are displayed and I
> decided to display the 7th one, a refresh call will be done (as onCalendarAdded
> will be posted) and getItems() will be invoked on the composite calendar, for
> all views. This will generate 7 getItems() calls per view.
I can confirm this; we want this and will block the next release on a solution.

A fix for this should also solve the 'flickering event boxes' issue.

> Lightning has the agenda list box, minimonth view, task view, month view and
> multiday view. For those, it would generate at least 140 getItems() calls...
> followed by relayouting!
If I got Berend right, he's decoupled decoupled the views recently to not do that anymore (trunk).

> The idea behind the patch attached to this bug report is to:
> 
> 1- not refresh an undisplayed view
Is this really the case?

> 2- not refresh when doing batch operations
I think this is the way to go. The only problem I have (in general) with using the batch notification is that we thoroughly need to keep track that endBatch() is called, else we would end up in locked views. Moreover overlapping batch calls may delay views longer than we want, e.g.

<---- batch 1 ---->
      <---- batch 2 ---->

Comment 5

[Ludovic Marcotte]

(In reply to comment #4)
[snip]
> I think this is the way to go. The only problem I have (in general) with using
> the batch notification is that we thoroughly need to keep track that endBatch()
> is called, else we would end up in locked views. Moreover overlapping batch
> calls may delay views longer than we want, e.g.
> 
> <---- batch 1 ---->
>       <---- batch 2 ---->

Yep - what do you suggest for this? One thing we could do is to keep track of batch operations when it's not actually the user that asks for stuff.

That or at some point, we really make sure that we reset the counters to 0 and refresh the views - whatever the value was.

Comment 6

[Berend Cornelius]

I am just about to port the patch to the trunk code-line. I admit I have not yet compiled and tested the result, but I got stuck at the following codelines:

>       <method name="refresh">
>         <body><![CDATA[
>-          var refreshJob = {};
>-          this.mRefreshQueue.push(refreshJob);
>-          this.popRefreshQueue();
>+	  if (this.mBatchCount == 0 && currentView() &&
>+	    (currentView().nodeName == "calendar-decorated-day-view" ||
>+	    currentView().nodeName == "calendar-decorated-week-view")) {
>+
>+	      var refreshJob = {};
>+	      this.mRefreshQueue.push(refreshJob);
>+	      this.popRefreshQueue();
>+	  }

Is it not necessary to set some kind of flag when the view is not displayed so that  it is guaranteed that the refresh is caught up when the view gets the active one?

Comment 7

[Ludovic Marcotte]

(In reply to comment #6)
> Is it not necessary to set some kind of flag when the view is not displayed so
> that  it is guaranteed that the refresh is caught up when the view gets the
> active one?

There seems to be a "checked" attribute set in switchToView() from calendar-views.js.

Comment 8

[Berend Cornelius]

Attachment: patch v. #2

I have converted the patch of ludovic to trunk and me and Andreas tested it. It works fine and it certainly fixes some issues. Yet I could not reproduce the behaviour of this bug description and also could not detect a great performance improvement. Andreas measured a slight performance improvement.
I find this issue a good completion of my changes in 
447683: Use full weekday name in Day and Week views' captions
where Window-resizing is observed only by the active calendar view calendar view.

Comment 9

[Berend Cornelius]

in reply to comment #7:
Yes you are right. It is guaranteed, that the current calendarView is refreshed as it should when it is becoming visible.

Comment 10

[Ludovic Marcotte]

(In reply to comment #8)
> improvement. Andreas measured a slight performance improvement.

In order to really see what it does, try this:

		var composite = getCompositeCalendar();

		for (var i = 0; i < calendarListTreeView.rowCount; i++) {
			var calendar = calendarListTreeView.getCalendar(i);
			composite.addCalendar(calendar);
			calendarListTreeView.treebox.invalidateRow(i);
		}

then, wrap this around a composite.startBatch() / composite.endBatch().

We have a feature like this in SOGo Integrator in order to show all calendars / only the selected one rapidly.

Comment 11

[Ludovic Marcotte]

One thing that would also be nice is to wrap the replayChanges() for cached calendars in startBatch()/endBatch() calls. Something like this, from calCachedCalendar.js

        if (this.supportsChangeLog) {
            LOG("[calCachedCalendar] Doing changelog based sync for calendar " + this.uri.spec);
	    var thisCalendar = this;
	    this.mUncachedCalendar.startBatch();
            var opListener = {
                onResult: function(op, result) {
		    thisCalendar.mUncachedCalendar.endBatch();
                    if (!op || !op.isPending) {
                        var status = (op ? op.status : Components.results.NS_OK);
                        ASSERT(Components.isSuccessCode(status), "replay action failed: " + (op ? op.id : "<unknown>"));
                        LOG("[calCachedCalendar] replayChangesOn finished.");
                        emptyQueue(status);
                    }
	      }
            };
            this.mPendingSync = this.mUncachedCalendar.replayChangesOn(this.mCachedCalendar, opListener);
            return this.mPendingSync;
        }

This dramatically reduces Lightning's startup speed with many CalDAV cached calendars and it also speeds up alot calendars refresh operations.

Comment 12

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 353462 [details] [diff] [review]
patch v. #2

>diff --git a/calendar/base/content/agenda-listbox.js b/calendar/base/content/agenda-listbox.js
>--- a/calendar/base/content/agenda-listbox.js
>+++ b/calendar/base/content/agenda-listbox.js
>@@ -440,6 +440,9 @@ function enableAgendaPopupMenu(aPopupMen
> 
> agendaListbox.refreshCalendarQuery =
> function refreshCalendarQuery(aStart, aEnd) {
>+    if (this.mBatchCount) {
>+        return;
>+    }
I'd prefer more explicitly checking |this.mBatchCount > 0|.

>+      <method name="isVisible">
>+        <body><![CDATA[
>+            return (this.nodeName == currentView().viewElem.nodeName);
>+        ]]></body>
>+      </method>
Why not (this == currentView().viewElem) ? Also, consider making this a readonly property instead of a method.


> 
>         switch (aCol.id) {
>             case "calendar-list-tree-checkbox":
>+                composite.startBatch();
>                 if (composite.getCalendar(calendar.uri)) {
>                     composite.removeCalendar(calendar.uri);
>                 } else {
>@@ -468,6 +469,7 @@ var calendarListTreeView = {
>                 break;
>         }
>         this.treebox.invalidateRow(aRow);
>+        composite.endBatch();
>     },
You are alwyas ending the batch, but only starting it for the checkbox column, why?
Also, make sure that no calls are made that could cause an exception (and therefore never end the batch).


r=philipp with comments considered.

Comment 13

[Berend Cornelius]

Addressed the comments of philipp and 
pushed patch to comm-central:

http://hg.mozilla.org/comm-central/rev/5051123029dc

-> fixed!

Comment 14

[Ludovic Marcotte]

Berend, if this bug is marked as fixed, we should also mark #469605 as fixed.

Also, what about my comment #11 ?

Comment 16

[Berend Cornelius]

>Also, what about my comment #11 ?

I have already pointed Daniel to this comment and he will think a about it.

Comment 17

[Jonathan Harker]

I suspect there is a regression introduced somewhere recently where this is fix is no longer working. Every time I add an event, the view immediately refreshes before I get the Edit New Event dialog, and a "New Event" event is added. This is very annoying. Ubuntu 11.04, 11.10, 12.04, the last few releases since at least February this year, currently Lightning 1.5.2

Even when I set my calendars to Manual refresh, the calendars refresh every time I add, edit or move an event. I have 7 calendars (CalDAV, davical) and a Google calendar set up.