Closed Bug 469691 Opened 11 years ago Closed 11 years ago

Unnecessary refresh of views

Categories

(Calendar :: Calendar Views, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: lmarcotte, Assigned: berend.cornelius09)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Initial version (obsolete) — Splinter Review
Attachment #353059 - Attachment is patch: true
Attachment #353059 - Attachment mime type: application/octet-stream → text/plain
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.
Flags: wanted-calendar1.0?
Flags: blocking-calendar1.0?
OS: Mac OS X → All
If this gets confirmed, this is clearly a 1.0 blocker.
Flags: wanted-calendar1.0?
(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 ---->
Assignee: nobody → lmarcotte
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking-calendar1.0? → blocking-calendar1.0+
Hardware: PC → All
(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.
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?
(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.
Attached patch patch v. #2Splinter Review
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.
Assignee: lmarcotte → Berend.Cornelius
Attachment #353059 - Attachment is obsolete: true
Attachment #353462 - Flags: review?(philipp)
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.
(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.
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.
Attachment #353462 - Flags: review?(philipp) → review+
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.
Addressed the comments of philipp and 
pushed patch to comm-central:

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

-> fixed!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Berend, if this bug is marked as fixed, we should also mark #469605 as fixed.

Also, what about my comment #11 ?
Duplicate of this bug: 469605
>Also, what about my comment #11 ?

I have already pointed Daniel to this comment and he will think a about it.
Target Milestone: 1.0 → 1.0b1
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.
You need to log in before you can comment on or make changes to this bug.