710351 - Full sync providers like ICS very slow when refreshing

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v1

With the cache enabled, full sync providers pull all items into the storage cache in a loop that is done on the main thread without relief. This needs to be done asynchronously, or at least with a way to process other events inbetween.

Using processNextEvent() has proven to not be reliable, but there are alternatives. This path is work in progress and currently duplicates the events in the unifinder. I haven't found out why though.

On the plus side, this makes Lightning very usable with an ics cached calendar.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I found out whats happening here:

On a reload, the unfinder triggers a getItems call to refill the unifinder with all items. The cache layer then goes about refreshing and starts adding the items to the storage calendar and notifies the unifinder when this is complete, which adds the first batch of all items to the unifinder. Until now everything is ok.

The storage calendar then starts triggering a lot of onAddItem calls, because the items are being added from the cache. These calls are passed on to the unifinder and it starts adding those items in again.

This behavior is only triggered with the above patch applied, since the onOperationComplete and additions are not done synchonously anymore. 

Now how to fix this. The presumably easy way is a 3-line hack in the unifinder that makes sure that items that are already in it are not added again. This works fine on the UI side, although I have not tested what happens if a delete is done before the onAddItem call fires. I'd guess its wrongly added back in.

The better way would be quite tricky, since it involves instructing the storage calendar to add an item without triggering an onAddItem call. This breaks the calICalendar interface since we would specialcase just for the storage calendar.

I'm going to experiment with disabling the observer for the time of the full refresh and then firing the onLoad as usual. That should get rid of the onAddItem calls, but it might have other side effects.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Turns out that the double events in the unifinder also happen without the patch! Aside from fixing a typo I think we can take v1 of this patch.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

This patch takes care, although it only really makes sense when bug 717067 is fixed. I'd appreciate some testing and if it works out, we can consider using cal.forEach in other places too.

Comment 4

[Matthew Mecca [:mmecca]]

Comment on attachment 587523 [details] [diff] [review]
Fix - v1

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

Looks good. r=mmecca

::: calendar/base/content/calendar-unifinder.js
@@ +496,5 @@
> +       /*for each (let item in aItemArray) {
> +            if (!(item.hashId in this.eventIndexMap)) {
> +                this.eventArray.push(item);
> +            }
> +        }*/

Test code for bug 717067 ? Also there are a couple of stray whitespace changes in this file.

::: calendar/base/modules/calIteratorUtils.jsm
@@ +101,5 @@
> +                while (((new Date()).getTime()  - startTime) < LATENCY) {
> +                    let next = ourIter.next();
> +                    let rc = body(next);
> +                    if (rc == cal.forEach.BREAK) {
> +                        throw StopIteration;

It looks like the completed() function will be called even if we break out early with cal.forEach.BREAK? Maybe that could be clarified in the function comments.

Comment 5

[Mohit Kanwal [:redDragon]]

Comment on attachment 587523 [details] [diff] [review]
Fix - v1

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

I gave this a quick test and had no issues.  I only tested with my own local ics file and a read-only ics copy from my google account. Maybe a test with a huge ics file is needed :)

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset 050136c0cfc3