Closed Bug 502936 Opened 10 years ago Closed 8 years ago

cached calendars should not be refreshed at launch/startup time

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: WSourdeau, Assigned: redDragon)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, Whiteboard: [needed beta][no l10n impact])

Attachments

(1 file, 8 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; fr; rv:1.9.0.11) Gecko/2009061317 Iceweasel/3.0.11 (Debian-3.0.11-1)
Build Identifier: 20090707041124

When calendars are cached, a getItems is performed on them when the views become available and again when the calendar triggers the "onLoad" notification. The attached patch prevent getItems() operations on such calendars as long as the "onLoad" has not been posted. This enhances responsitivity and load time when a couple of cached calendars are configured.

Reproducible: Always
Attached patch the fix (obsolete) β€” β€” Splinter Review
For a single calendar, this patch reduces the amount of calls to "getItems" on the storage instance goes from 29 down to 8.
Attachment #387291 - Flags: ui-review?(philipp)
Assignee: nobody → WSourdeau
Status: UNCONFIRMED → ASSIGNED
Component: Calendar Views → Internal Components
Ever confirmed: true
OS: Linux → All
QA Contact: views → base
Hardware: x86 → All
Attachment #387291 - Flags: ui-review?(philipp) → review?(philipp)
Related to bug 412914?
Attachment #387291 - Flags: review?(philipp) → review-
Comment on attachment 387291 [details] [diff] [review]
the fix

This looks like a hack. You should postpone/queue getItems calls until the calendar is loaded (like ics is doing); the caching facade might be the right place.
(In reply to comment #2)
> Related to bug 412914?

Probably one of the causes yes. This bug and #500718 are both related actually.
(In reply to comment #3)
> (From update of attachment 387291 [details] [diff] [review])
> This looks like a hack. You should postpone/queue getItems calls until the
> calendar is loaded (like ics is doing); the caching facade might be the right
> place.

The problem is that getItems on storage calendars is a long operation. Postponing calls to getItems will not help because we want to reduce them.
As far as I understand, the "onLoad" event is meant to announce the availability of a calendar, or the availability of new data. Therefore, I think no getItems should occur at all on a calendar as long as the "onLoad" was not triggered for that calendar.
> The problem is that getItems on storage calendars is a long operation.
> Postponing calls to getItems will not help because we want to reduce them.
> As far as I understand, the "onLoad" event is meant to announce the
> availability of a calendar, or the availability of new data. Therefore, I think
> no getItems should occur at all on a calendar as long as the "onLoad" was not
> triggered for that calendar.

Maybe a new "onCalendarReady" notification should be created and taken into account for that case?
Attached patch enhanced version (obsolete) β€” β€” Splinter Review
This new version fixes the case where an onOperationComplete would not be triggered when no enabled calendar had emitted an initial "onLoad".
Attachment #387291 - Attachment is obsolete: true
Attachment #387459 - Flags: review?(dbo.moz)
(In reply to comment #5)
> The problem is that getItems on storage calendars is a long operation.
> Postponing calls to getItems will not help because we want to reduce them.
> As far as I understand, the "onLoad" event is meant to announce the
> availability of a calendar, or the availability of new data. Therefore, I think
> no getItems should occur at all on a calendar as long as the "onLoad" was not
> triggered for that calendar.

Right, but you need to keep the API. If a calendar is enabled, it needs to respond to its getItems calls when data has loaded. I think it's wrong to break this API.
I think the core problem is that the views use the composite facade/multiplexer, acting as a single calendar, thus the views will retrieve the same data multiple times when onload events occur. I think the views need to deal with the individual calendars to optimize that. calCompositeCalendar needs to adhere calICalendar..
(In reply to comment #6)
> Maybe a new "onCalendarReady" notification should be created and taken into
> account for that case?
Where's the difference to the current "onload" hint/event?
(In reply to comment #8)

> Right, but you need to keep the API. If a calendar is enabled, it needs to
> respond to its getItems calls when data has loaded. I think it's wrong to break
> this API.

We probably should prevent getItems from the views themselves then... That's where it's a bit tricky.

> I think the core problem is that the views use the composite
> facade/multiplexer, acting as a single calendar, thus the views will retrieve
> the same data multiple times when onload events occur. I think the views need
> to deal with the individual calendars to optimize that. calCompositeCalendar
> needs to adhere calICalendar..

What about another approach... if you move any refresh/reload handling from the views and transfer them exclusively to the calCompositeCalendar, the latter can then become a controller for the different views. I think the paradigm would be consistent and help with this problem as well as the problem where an onLoad triggers the refresh on ALL calendars, no matter what calendar the onLoad originated from. It would also offer an other approach to #500718. What do you think?
(In reply to comment #9)
> (In reply to comment #6)
> > Maybe a new "onCalendarReady" notification should be created and taken into
> > account for that case?
> Where's the difference to the current "onload" hint/event?

Well, it would separate the concept of calendar initialization (some would trigger it from their constructor while others from a callback) from the concept of data refresh (aka "changelog").
Any further comment about this? We are ready to put some effort into improving all of this but we need your opinion so that we know if we are on the right track... (AND to know when we can start working on this)
(In reply to comment #10)
> (In reply to comment #8)
...
> > I think the core problem is that the views use the composite
> > facade/multiplexer, acting as a single calendar, thus the views will retrieve
> > the same data multiple times when onload events occur. I think the views need
> > to deal with the individual calendars to optimize that. calCompositeCalendar
> > needs to adhere calICalendar..
> 
> What about another approach... if you move any refresh/reload handling from the
> views and transfer them exclusively to the calCompositeCalendar, the latter can
> then become a controller for the different views. I think the paradigm would be
> consistent and help with this problem as well as the problem where an onLoad
> triggers the refresh on ALL calendars, no matter what calendar the onLoad
> originated from. It would also offer an other approach to #500718. What do you
> think?

Could you please elaborate a bit more on this? Why does it help? Does such a controller need to implement calICalendar? Is is sensible to stick to that interface at all for the purpose of loading the views? Thanks!
I think all of the above can be ignored or postponed.

I have found a way to achieve the same result without modifying anything in the architecture, by removing all the calls to refresh() from the onLoad and the onEndBatch callbacks. I am going to send another patch a bit later. The gain in display performance is simply astonishing.
Attached patch refresh killer (obsolete) β€” β€” Splinter Review
This is my patch for Lightning 0.9 (Inverse edition). It's only here as a request for comment. Once everyone agrees, I'll adapt it to Lightning 1.0.

Meanwhile, I think this bugreport could be closed. Also, this patch follows the same mindset as 500718. Ultimately, I think it obsoletes the calCompositeCalendar altogether, at least as a calendar-proxy.

Please review it and submit your comments. I necessary, I'll close or rename this bugreport and open a new one a bit more representative of what was solved here.
Attachment #389767 - Flags: review?(dbo.moz)
Any update on the review for this patch?

It looks to me it should be a rather high-priority item. I've tested yesterday TB3b3 together with a Lightning nightly build with sogo-demo.inverse.ca (sogo1 account) and it was just totally unusable due to the major slowness of the refresh code.

I've then tested with our patch on our Inverse Edition and it was just _fast_.

We just need your input in order to move forward with a real patch for 1.0. I think it should also be a release blocker.
I'm fine with blocking for 1.0 (final at latest) since this patch has a driver. I can't say anything about the review right now, Daniel will surely comment soon.
Flags: blocking-calendar1.0+
Whiteboard: [not needed beta][no l10n impact]
3 weeks and still no updates?
Wolfgang, since you seem to have deeply dived into the batch handling code (and your patch resolves some of it), could you think about an alternative to it? We are encountering problems w.r.t. threading, see bug 494783 comment #26.
I don't deeply know the issue at hand here but I have two ideas:
- is batch mode still useful (after my patch)? It not, drop it.
- if we need to keep it, wouldn't there be a way for the new parsing code to invoke the callbacks from the calling thread?

Besides, do you have any comment one my code at all?
(In reply to comment #20)
> I don't deeply know the issue at hand here but I have two ideas:
> - is batch mode still useful (after my patch)? It not, drop it.

My guess is that batch mode is still useful but only on the backend side, not the views. The rest can work properly with a getItems at initialization and onAdd/onModify/onDelete.
Daniel, any update on the review here?
Comment on attachment 387459 [details] [diff] [review]
enhanced version

r- for the mentioned reasons. Moreover, mixing caching conditionals into composite seems to be the wrong approach to me.
Attachment #387459 - Flags: review?(dbo.moz) → review-
Comment on attachment 389767 [details] [diff] [review]
refresh killer

Wolfgang, could you please add a clean patch against comm-central?
In general I am fine with removing batch handling from agenda or unifinder code if it improves performance. It certainly simplifies the code.
Attachment #389767 - Flags: review?(dbo.moz)
Any updates on this bug?
Unless there are minimally invasive changes we can make here, I think this bug should be postponed to after 1.0. Given our test coverage is far from optimal, changing such a fundamental part of the calendar interfaces sounds very risky to me.

Wolfgang, Ludovic, is that ok with you?
Our GSoC student is currently working on this. He's porting the patch to 1.0b2 (Inverse Edition) and then, he'll port it to trunk.

Is 1.0b5 being scheduled here or the next version is 1.0?

In any cases, this patch MUST go in the next version.
1.0b5 is a maintenance release for 1.0b4 to fix some urgent issues, and is due towards the weekend. The real "next release" is hopefully 1.0 and given this is part of the soc project I'm happy to block 1.0 for this.
Assignee: WSourdeau → mohit.kanwal
Ok, looks good to me then.
Hi everyone, 

Its my pleasure to work on this. 

My summer is turning out to be more fruitful than I thought. =)
Attached patch Patch ported to Lightning (obsolete) β€” β€” Splinter Review
Patch ported to trunk. And tested.
Looks good so far.
Attachment #387459 - Attachment is obsolete: true
Attachment #389767 - Attachment is obsolete: true
Attachment #550731 - Flags: review?(philipp)
Comment on attachment 550731 [details] [diff] [review]
Patch ported to Lightning

Review of attachment 550731 [details] [diff] [review]:
-----------------------------------------------------------------

I've given this bug a test and unfortunately hit some problems. Some of it may just require explaining to me why its happening, but I think the reload remote calendar issue needs to be fixed. I'm going to have to deny review for this version of the patch.

::: calendar/base/content/agenda-listbox.js
@@ -918,4 @@
>  };
>  
>  agendaListbox.calendarObserver.onLoad = function() {
> -    this.agendaListbox.refreshCalendarQuery();

Same as in the month view: removing this breaks reload remote calendars

@@ -925,5 @@
>  function observer_onAddItem(item)
>  {
> -  if (this.mBatchCount) {
> -      return;
> -  }

Could you explain why removing batched mode improves things? What it was supposed to do is make sure that multiple changes do not hammer the UI, but I can see that doing a full refresh afterwards isn't right unless maybe there was a substantial amount of changes.

Anyway, I see two options here:

a) Get rid of batch mode altogether (this also means the whole onStartBatch/onEndBatch observer notification) because you think its drawbacks outweigh its merits.

b) Fix batch mode to do what it really should (i.e provide a mechanism for consumers of the batch mode to query up the changes and then do them once when batch mode ends.

::: calendar/base/content/calendar-base-view.xml
@@ +138,2 @@
>              },
>  

Removing this causes the following error:

1. Create an event (i.e on an ICS file:// calendar)
2. Remove the event externally (i.e with a text editor)
3. Reload remote calendars

Expected:
* The event should go away

Actual:
* The event stays around

@@ +229,4 @@
>                      case "readOnly":
>                      case "disabled":
>                          // XXXvv we can be smarter about how we handle this stuff
> +                        //this.calView.refresh();

I fear its not quite that easy. This breaks the following:

1. Create some events in your view
2. Mark the calendar disabled

Expected:
* The events should go away

Actual:
* The events stay around.


It does seem to work for readOnly though, at least from simple testing.

::: calendar/base/content/calendar-month-view.xml
@@ -1053,5 @@
>              for each (let itemData in boxItemData) {
>                let item = itemData.item;
> -              if (!deleteHash[item.hashId] && item.calendar.id == aCalendar.id) {
> -                deleteHash[item.hashId] = true;
> -                this.doDeleteItem(item);

Go ahead and do the same for calendar-multiday-view, the deleteHash variable is not needed.

@@ +1076,4 @@
>            let boxes = this.findDayBoxesForItem(aAlarmItem);
>            for each (var box in boxes) {
>              for each (var itemData in box.mItemData) {
> +              if (itemData.item.hasSameIds(aAlarmItem) && itemData.box) {

This change is no longer needed since bug 424808.

::: calendar/base/content/calendar-unifinder.js
@@ -126,5 @@
>              unifinderTreeView.clearItems();
>          }
> -        if (!this.mInBatch) {
> -            refreshEventTree();
> -        }

Same as in the month view: removing this code breaks the reload remote calendars functionality.
Attachment #550731 - Flags: review?(philipp) → review-
Attached patch De-bitrotted patch (obsolete) β€” β€” Splinter Review
For your convenience I've de-bitrotted the patch so it applies to the latest comm-central default branch.
Attachment #550731 - Attachment is obsolete: true
> >  
> >  agendaListbox.calendarObserver.onLoad = function() {
> > -    this.agendaListbox.refreshCalendarQuery();
> 
> Same as in the month view: removing this breaks reload remote calendars

I will let Mohit answer that one as he is assumed to have understood what he was working for...

> 
> @@ -925,5 @@
> >  function observer_onAddItem(item)
> >  {
> > -  if (this.mBatchCount) {
> > -      return;
> > -  }
> 
> Could you explain why removing batched mode improves things? What it was
> supposed to do is make sure that multiple changes do not hammer the UI, but
> I can see that doing a full refresh afterwards isn't right unless maybe
> there was a substantial amount of changes.

I can more or less answer that one. Batch mode seems to have been implemented in the views in order to avoid progressive invocations of the onAddItem/onModifyItme and onDeletedItem callbacks, by preventing them from doing anything. This, we want to get rid of. The other aspect of batch mode is to enable transactional updates of the SQLite databases, which generally improves performances, in particular on Windows. That aspect we want to keep.

Why we want to get rid of it in the views is for the following reason: we want to get rid of the call to getItems in the "onLoad" method because that's the original culprit, in that it is invoked each time any calendar is done refreshing, whether any change occurred and because it forces a complete redraw of the current view when done. Instead we want to make use (again?) of the onAddItem/onDeleteItem and onModifyItems methods because they are invoked only when something actually happens in the backend, only by item, by calendar. This way, we only draw or redraw the modified events and only once per event.

Therefore....

> a) Get rid of batch mode altogether (this also means the whole
> onStartBatch/onEndBatch observer notification) because you think its
> drawbacks outweigh its merits.

Yes, we should, unless there was any other reason for the batch handling in the views (see comment #20 above), but only in the views.

> b) Fix batch mode to do what it really should (i.e provide a mechanism for
> consumers of the batch mode to query up the changes and then do them once
> when batch mode ends.

We only have that, as the onAdd/MOdify/DeleteItem are invoked only when needed. So there is no real use for this.

For the rest, I will leave the fixes to Mohit, as, again, he is supposed to have understood what he did and why he did it...
> We only have that, as the onAdd/MOdify/DeleteItem are invoked only when
> needed. So there is no real use for this.

Sorry, typo -> "We only" = "We already"
Hi Philipp and Wolfgang, 

Finally I have the time to reply to you 

(In reply to Philipp Kewisch [:Fallen] from comment #32)
> ::: calendar/base/content/agenda-listbox.js
> @@ -918,4 @@
> >  };
> >  
> >  agendaListbox.calendarObserver.onLoad = function() {
> > -    this.agendaListbox.refreshCalendarQuery();
> 
I removed this because essentially it was becoming redundant with the batch mode with onEndBatch being fired after a onLoad notification. Hence the views were getting refreshed multiple times. And it was not causing an issue with calendar reload or loss of information wrt events. 

I went inside batch mode and tried to remove all batch mode invocations so that the views don't call it, however, there were still parts for example the tasks view pane in the today-pane that gets refreshed on a batch mode invocation, so I had to keep it. If there is a solution to that issue then we can completely do away with the batch mode and its notifications. 

The previous patch(proposed by Wolfgang) had the same changes, I mirrored them to fit the new code which has undergone a lot of changes after Lightning 0.9

Let me know how should I proceed with this.

Cheers
Mohit
Keywords: perf
Summary: cached calendars should not be refreshed at launch time → cached calendars should not be refreshed at launch/startup time
This patch fixes the reload remote calendar functionality on disabling a calendar. And removed delete hash from multiday-view.
Attachment #551233 - Attachment is obsolete: true
Attachment #552051 - Flags: review?(philipp)
Comment on attachment 552051 [details] [diff] [review]
version 3 of the patch with reload remote calendar fixed

Review of attachment 552051 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to have to set r- here, there are again a few issues, as described below.

::: calendar/base/content/agenda-listbox.js
@@ -922,1 @@
>  };

Still having trouble here. STR:

1. Create an event on an ics calendar that happens tomorrow
2. See it in the today pane
3. Externally change the title of the event
4. Reload remote calendars

Expected:
* Title should be updated 

Actual:
* Title is not changed, neither in the view nor in the today pane.

::: calendar/base/content/calendar-base-view.xml
@@ -141,1 @@
>              },

This is probably the culprit for the views not reloading as described above.

::: calendar/base/content/calendar-task-tree.xml
@@ -844,5 @@
>  
>            onLoad: function tTO_onLoad() {
> -              if (this.mBatchCount == 0) {
> -                  this.binding.refresh();
> -              }

I would have expected this to fail similar to the views above, but surprisingly reloading tasks works! :-)

::: calendar/base/content/calendar-unifinder.js
@@ -131,5 @@
>      },
>  
>      onAddItem: function uO_onAddItem(aItem) {
>          if (isEvent(aItem) &&
> -            !this.mInBatch &&

Since you are no longer using mInBatch, I'd appreciate if you could remove it from the observer alltogether. Similar in other places, if you no longer use a variable then please remove it.
Attachment #552051 - Flags: review?(philipp) → review-
Comment on attachment 552051 [details] [diff] [review]
version 3 of the patch with reload remote calendar fixed

Thank you splinter reviews for mushing it all up!


> agendaListbox.calendarObserver.onLoad = function() {
>-    this.agendaListbox.refreshCalendarQuery();
> };
These are the lines of the first issue I meant.

>diff -r c63fefad39c0 calendar/base/content/calendar-base-view.xml
>--- a/calendar/base/content/calendar-base-view.xml	Sun Aug 07 02:22:19 2011 +0800
>+++ b/calendar/base/content/calendar-base-view.xml	Wed Aug 10 20:24:26 2011 +0800
> 
>             onLoad: function onLoad() {
>-                this.calView.refresh();
>             },
And these are the second lines, the culprit in the views.
Attached patch Patch-Version4 (obsolete) β€” β€” Splinter Review
Hi all,

In this version, i tried to address the above concern and also removed batch variables from calendar-task-tree. I have done testing on caldav calendars and it works so far.

For ICS, I am not sure how the behavior will be since the icsParser worker thread keeps crashing under the newer comm-central repo. Can someone test it out on ICS provider ?

Thanks
Mohit
Attachment #552051 - Attachment is obsolete: true
Attachment #554147 - Flags: review?(philipp)
Comment on attachment 554147 [details] [diff] [review]
Patch-Version4

Review of attachment 554147 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Mohit, are you sure you uploaded the right version? I see a few batch variables left in the files, see below. Don't worry about the ics issues, I'll try to fix those soon. If you need to test this in the meanwhile you'll have to build comm-aurora, but that might break other things so don't worry about it.

Since you're almost there I'm setting review+, but please upload a new patch with the batch variables removed (also look out for any definitions of the variables, make sure grep -r mBatchCount calendar/* doesn't return any relevant results)

::: calendar/base/content/agenda-listbox.js
@@ +911,3 @@
>  agendaListbox.calendarObserver.onEndBatch =
>  function() {
>      this.mBatchCount--;

Is this variable used anywhere else? Maybe you can remove it too?

::: calendar/base/content/calendar-base-view.xml
@@ +131,3 @@
>              },
>              onEndBatch: function onEndBatch() {
>                  this.calView.mBatchCount--;

Same here

::: calendar/base/content/calendar-task-tree.xml
@@ +837,3 @@
>  
>            onEndBatch: function tTO_onEndBatch() {
>                this.mBatchCount--;

Same here. Please also check any other batch variables you are touching.
Attachment #554147 - Flags: review?(philipp) → review+
Hi Philipp, there are some functions like refreshCalendarQuery in agendaListbox.js which make use of the batch variables before doing a refresh query. For example :

agendaListbox.refreshCalendarQuery =
function refreshCalendarQuery(aStart, aEnd, aCalendar) {
    if (this.mBatchCount > 0) {
        return;
    }

Should I remove these statements as well? It sort of serves as an additional guard against refresh during batch operations from the views.
(In reply to Mohit Kanwal [:redDragon] from comment #42)

> Should I remove these statements as well? It sort of serves as an additional
> guard against refresh during batch operations from the views.

I'd be ok with that, but please do test for negative effects when you do.
Whiteboard: [not needed beta][no l10n impact] → [needed beta][no l10n impact]
Attached patch Version[5] (obsolete) β€” β€” Splinter Review
Cleaned up the patch with removal of mBatchCount variables. The only place they exist now is calProviderUtils.jsm which is local to the module and is responsible for firing notifications "onEndBatch" and "onStartBatch". I don't want to remove mBatchCount from calProviderUtils since maybe later people might want to do some stuff based on the notifications. 

Apart from that there are no negative effects and the speed gain after applying this patch is simply astonishing. One can clearly see before applying the patch that the views refresh twice on a "reload remote calendar" command, but they only refresh once after applying the patch.
Attachment #554147 - Attachment is obsolete: true
Attachment #556762 - Flags: review?(philipp)
Comment on attachment 556762 [details] [diff] [review]
Version[5]

Review of attachment 556762 [details] [diff] [review]:
-----------------------------------------------------------------

r=philipp, but I'd appreciate if you could answer these two questions before I push the patch:

::: calendar/base/content/calendar-unifinder.js
@@ +113,1 @@
>          refreshEventTree();

Is refreshEventTree() needed here?

::: calendar/base/content/widgets/minimonth.xml
@@ +642,5 @@
>        <method name="onLoad">
>          <parameter name="aCalendar"/>
>          <body><![CDATA[
> +            this.resetAttributesForDate();
> +            this.getItems();

Could you explain this change? Not saying its wrong, but I wonder why this hasn't been there before?
Attachment #556762 - Flags: review?(philipp) → review+
(In reply to Philipp Kewisch [:Fallen] from comment #45)
Hi Philipp,

> ::: calendar/base/content/calendar-unifinder.js
> @@ +113,1 @@
> >          refreshEventTree();
> 
> Is refreshEventTree() needed here?

Yes refreshEventTree() is needed here, otherwise, whenever a "onAddItem" notification occurs, two copies of the same event get added to the unifinder.

> 
> ::: calendar/base/content/widgets/minimonth.xml
> @@ +642,5 @@
> >        <method name="onLoad">
> >          <parameter name="aCalendar"/>
> >          <body><![CDATA[
> > +            this.resetAttributesForDate();
> > +            this.getItems();
> 
> Could you explain this change? Not saying its wrong, but I wonder why this
> hasn't been there before?

Yes I don't know how this slipped through, sorry about that, I think must have happened when I was reconciling Inverse's patch for refresh-killer.

I am uploading a new patch with the lines removed.
Attached patch [Patch Version-6] β€” β€” Splinter Review
Removed the lines in the onLoad handler of minimonth.xml
Attachment #561207 - Flags: review?(philipp)
Attachment #556762 - Attachment is obsolete: true
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/1c7e6fcdc4fa>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Backported to comm-aurora <http://hg.mozilla.org/releases/comm-aurora/rev/f49d83800dff>
Target Milestone: Trunk → 1.0b8
Backported to comm-beta <http://hg.mozilla.org/releases/comm-beta/rev/347508f378fd>
Target Milestone: 1.0b8 → 1.0b7
Attachment #561207 - Flags: review?(philipp) → review+
You need to log in before you can comment on or make changes to this bug.