Closed Bug 393817 Opened 17 years ago Closed 16 years ago

CalDAV provider is too chatty

Categories

(Calendar :: Provider: CalDAV, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: browning, Assigned: browning)

References

Details

Attachments

(1 file, 3 obsolete files)

The CalDAV provider currently issues a REPORT request including calendar-data on every view change, refresh, etc., meaning that it refetches and reparses the same data over and over. It would be much smarter for it to cache (a certain number of) items, and initially fetch etags in order to determine what items really need to be fetched & parsed instead of merely grabbed from cache. 

Taking this. Patch in progress which depends on code in the patches for 373370 and  393816, so I'm setting those a blockers
not requesting review pending resolution of 373370, 393816
Attached patch patch version 2, de-bitrotted (obsolete) — Splinter Review
patch adds a memory cache so that getItems() can fetch from the server, and parse, only new/changed items; unchanged items are fetched from cache as needed.

full disclosure: there's a small leak here. When accessing a shared calendar, if an item is deleted by another client we'll stop displaying it but it won't be removed from the cache. There's code in this patch to remove it from cache when the original query was for both VEVENTs and VTODOs, without timerange restriction; unfortunately that's not a query we currently ever make. I expect to rewire CalDAV refresh() to issue such a request (so that it doesn't issue multiple redundant requests as it does now) but it seemed appropriate to me to do that in another bug.
Attachment #278718 - Attachment is obsolete: true
Attachment #291350 - Flags: review?(ctalbert)
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All
Comment on attachment 291350 [details] [diff] [review]
patch version 2, de-bitrotted

This patch is causing some funky things to happen wrt recurring items; withdrawing review whilst I fix that.
Attachment #291350 - Flags: review?(ctalbert)
- add memory calendar for the UI to natter at
- rewire refresh() (which also get runs at calendar object creation) so that it does {get all etags && fetch deltas}, minimizing network traffic and item parsing
- remove the now-unneeded query-queuing code and instead serialize the two (VTODO, VEVENT) queries done by refresh(); this keeps bug 335462 from coming back and also fixes bug 395654

I haven't attempted to profile it, but clearly the first two points are a performance win. And the constant redundant queries are a common criticism of Sb/Ltn in CalDAVdom. Also, this will simplify certain aspects of fixing bug 409921. I'd like to see this in 0.8.
Attachment #291350 - Attachment is obsolete: true
Attachment #296063 - Flags: review?(daniel.boelzle)
Flags: wanted-calendar0.8?
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Blocks: 409921
How does this patch relate to the work in bug 393395? Is this change still needed when the offline changes are in?
As I see it, the basic problem here is that the CalDAV provider needs to have some kind of buffer between the multiple overlapping queries from the UI and the network i/o done by the provider; if I understand correctly the work being done in bug 393395 it is not (at least currently) going to be able to provide that. This patch uses a memory calendar. I suppose it would be possible in the future to use a the storage calendar in the calICachingCalendar instead - but that would need to wait until the calICachingCalendar is not experimental, and it would have performance implications I think. So I don't see a direct relation between this bug and 393395, at least given the approach being taken in the latter (which seems like a sensible one to me).
This version uses the memory calendar's new relaxedMode (as suggested by dbo via email) and thus does not need to touch calMemoryCalendar.js at all. No other changes.
Attachment #296063 - Attachment is obsolete: true
Attachment #300573 - Flags: review?(daniel.boelzle)
Attachment #296063 - Flags: review?(daniel.boelzle)
(In reply to comment #7)
> This version uses the memory calendar's new relaxedMode (as suggested by dbo
> via email) and thus does not need to touch calMemoryCalendar.js at all. No
> other changes. 
> 
I don't see a relaxedMode documented in calICalendar.idl. Can you provide a bug number where that was introduced / will be introduced?
That has been introduced with the offline works. Because the "relaxedMode" (at least for now) is an implementation detail of some providers (memory, storage), I don't think we should define it at calICalendar.
Comment on attachment 300573 [details] [diff] [review]
use memory calendar's relaxedMode

>     this.mItemInfoCache = [];
should be a plain object {} instead of array

>+        this.mMemoryCalendar.uri = this.mUri;
IMO there's no reason to set the URI at the memory calendar.

>+        this.mObserver = new calDavObserver(this);
no need to store the observer reference at the calendar instance, please use a local var.
>+    set uri(aUri) {
>+        this.mUri = aUri;
>+        this.initMemoryCalendar();
>+
>+        this.checkDavResourceType();
return aUri missing
>+    },

>+    refresh: function caldav_refresh() {
>+
>+        var itemTypes = new Array("VEVENT", "VTODO");
>+        var typesCount = itemTypes.length;
>+        var refreshEvent = {};
>+        refreshEvent.itemTypes = itemTypes;
>+        refreshEvent.typesCount = typesCount;
>+        refreshEvent.queryStatuses = [];
>+        refreshEvent.itemsNeedFetching = [];
>+        refreshEvent.itemsReported = [];
>+
>+        this.getUpdatedItems(refreshEvent);
>+
>+    },

I am missing an onLoad event being fired once the cache has been updated; supposed to be fired from reportInternal.

>             } else {
>                 LOG("Server does not support CalDAV scheduling.");
>-                if (thisCalendar.mAuthenticationStatus == kCaldavFreshlyAuthenticated
>-                    && thisCalendar.mPendingStartupRequests.length > 0) {
>-                    thisCalendar.popStartupRequest();
>-                }
>+                thisCalendar.refresh();
>             }
>         }

There are some more occurrences where refresh() is called directly. I can't foresee whether it is ok to fire onLoad events for all those cases or whether that will trigger unnecessary load (since onLoad triggers subsequent getItems calls). Maybe you need to spend a refresh() that doesn't fire onLoad.


Finally, well done, thanks for the patch and sorry for delay. I think this will speed up caldav and prepares it to fit for calCachedCalendar.

I haven't tested this patch, but I assume you have (since you have quite a comprehensive server test suite).

r=dbo with refresh logic fixed.
Attachment #300573 - Flags: review?(daniel.boelzle) → review+
Thanks, Daniel. I've made the first three changes; thanks for catching them. 

I also added an mObservers.notify(onLoad, [this]) to reportInternal's reportListener.onOperationComplete. It doesn't seem to make Sunbird behave any differently. Since reportListener.onOperationDetail uses the memoryCalendar's {modify|delete}Item to register changes - and since both of those notify observers - I think that maybe  it's just causing some extraneous getItems calls.

I'm not really sure what you would like to see fixed in the refresh() logic. While there are three instances where the patch has the provider calling refresh() directly, only one of these instances will get executed, once, per session, called by the URI setter (at some remove) as part of initialization. Since the usual startup getItems calls will now get redirected to the memoryCalendar, we need to call refresh() first to populate that cache. And since the initialization chain begins with some probing it's easiest to have three places where that refresh() might get called (calendar has scheduling, calendar does not have scheduling, calendar has error) which get called depending on probe results. (The calendar-has-error case is for attempts to recover from network-went-away-and-came-back and will need to be revisited once we have caching).

So... I'm happy to fix the refresh() logic if you can tell me a little more specifically what's broken about it so that I know how to fix it. I'm also curious as to what onLoad is adding on top of modifyItem/deleteItem. And I'm starting to wonder if it would be worthwhile to wrap reportInternal in startBatch/endBatch. :-)

Yes, I've tested this (branch mostly) against AppleDarwin, Bedework, Chandler, DAViCal, and Oracle. 
Thanks for the explanations, Bruno. I was just wondering whether we could avoid extraneous getItems calls when refreshing from within the provider, whether those refreshes really need to trigger onLoad events. If that's ok, then go for it.
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
build 2008021319
I've just tested it against DavICAL.
At startup ligthning downloads all calendars even those I've unchecked
and checks it so the become visible !!!
with the 20080212 or 20080210  it was not the case !
Offline (beta) support is no more working either
At start the CPU goes up 60%, the calendar view is full of all Caldav Calendar
It's no usable at all !!
If I can't help, please let me know
Maxime, see Bug 412914 and Bug 412963 for the offline cache issues. Otherwise I recommend to file new bug reports for new issues but please search for existing reports first.
(In reply to comment #17)
Bug 412914 : is when a calendar is full off events
Bug 412963 : is when the state of disabled calendar is no more saved.

My case is still different. even with the latest nightly build 20080217 where bug 417873 is fixed)
I Start lightning, all my calendars are unchecked first then one by one lightning download them, and checks the checkbox !!, I can see the checkboxes checked one by one as if a ghost was playing with my mouse !

The result is that my view are overcrowded by 15 calendars, lightning goes to 70% of my CPU, and I can use it anymore 

It is a very bad regression.
If I can help tell me

Sorry, Maxime, but I'm not understanding how what you describe is distinct from but 412963. That bug would probably be a better place to continue this discussion. The first data point that would interest me would be: do you still see this problem if you turn off calendar caching?
That's the major difference, the calendars are _not_ cached !
by the way, 2 guys of my company have reported to me the same pb.
And the other difference IMHO, is that at the very start all calendars are disabled so the  state is correctly store, but few seconds latter  lightning starts to download them and then checked the checkboxes (maybe it's the opposite order).
Is it more clear ?
I create a new bug for my pb : bug 418569
You need to log in before you can comment on or make changes to this bug.