optimize caldav queries for "long" calendars

RESOLVED FIXED in 0.9

Status

--
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: WSourdeau, Assigned: WSourdeau)

Tracking

unspecified
Dependency tree / graph
Bug Flags:
blocking-calendar0.9 -
wanted-calendar0.9 +
tb-integration +

Details

Attachments

(2 attachments, 12 obsolete attachments)

48.73 KB, patch
Fallen
: review+
browning
: review-
Details | Diff | Splinter Review
77.58 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; fr-FR; rv:1.8.1.14) Gecko/20080404 Iceweasel/2.0.0.14 (Debian-2.0.0.14-2)
Build Identifier: 0.8 2008051319

The CalDAV provider queries for events and tasks in two operations: one calendar-query, retrieving all the etags of all the events of the user's calendar, and another calendar-multiget, which fetches the calendar-data of most of the returned event HREFS.

Since there is no time-range specified, the amount of events is directly related to the time needed to parse and display them.

For a test calendar of 20 events, this is seamless. But when users have a calendar with more than 5000 events, the startup and refresh time of Thunderbird becomes unacceptably slow. Moreover, the parsing and rendering of all that data is blocking the application since most people don't have a Quad-Core XEON as a workstation.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.



Possible optimisations:
a) specify a time-range, similar to the one 0.7 used to do?
b) cache the information when changing views or timeframes: request the calendar-data only of NEW or MODIFIED events in the specified time-range (based on the etag)?
(Assignee)

Updated

11 years ago
Hardware: PC → All
making the title a bit more useful.
Summary: unacceptable and uselessly important queries → optimize caldav queries for "long" calendars

Comment 2

11 years ago
As I see it our main problem here is that the CalDAV provider is not yet properly wired up to the offline cache. Instead of reverting to the 0.7 behavior, I think we'd be better off saving the etag and path of the item along with the item in the cache (likely as X-props in the item) so that when we do the initial etag query we can follow it up with a multiget that fetches only deltas, not the entire datastore as we do now. 
(Assignee)

Comment 3

11 years ago
What I suggest is not a reverse to the 0.7 behaviour. I think the idea in 0.8 of using a cache really is interesting. But what mean is to build the etag and href database depending on the current view, and populate it as the user wanders through the views.

Two questions though:
What is your idea with the x-tag?
And if we do as I suggest, does the "memory calendar" component has a way of tracking the time ranges being taken into account? For example, I fetch a way with X events, I then switch to the monthly view, are there mechanisms where I can now that that X set of events is part of the Y set matching the monthly view?

Comment 4

11 years ago
(In reply to comment #3)

> Two questions though:
> What is your idea with the x-tag?

We currently have two uncoordinated local caches: the CalDAV provider's memoryCalendar, and the offline (sqlite) cache, which is new/experimental in 0.8 and not yet properly wired to the CalDAV provider. What I'm suggesting is that the offline cache could include the etag/href database as well as the calendar-data, and that on startup the CalDAV provider could load calendar-data into it's memoryCalendar, and etag/href information into its .mItemInfoCache array, from the offline cache before going to the net. That way,  when that first etag query returns we'd need to fetch only that remote information that had changed since our last session instead of the whole magilla.

> And if we do as I suggest, does the "memory calendar" component has a way of
> tracking the time ranges being taken into account? For example, I fetch a way
> with X events, I then switch to the monthly view, are there mechanisms where I
> can now that that X set of events is part of the Y set matching the monthly
> view?
> 

Not really. The memoryCalendar simply assumes that it always has access to the entire calendar in its .mItems array. Currently the CalDAV provider is simply loading the entire calendar into that array at startup and only going to the net for refresh()es and add/modify/delete, relying on the memoryCalendar for view/timerange switches. That makes 0.8 - once the initial load has happened - it's enormously faster than 0.7. But clearly we need to find a way to make that initial load faster for calendars with larger event counts.

It seems to me that offline operation with an incomplete copy of the calendar is going to be problematic; that's what concerns me about populating the cache as the user wanders through the views (and is why I referred to it as reverting to the 0.7 behavior; I realize you weren't suggesting a wholesale reversion): if we only collect calendar-data as the user wanders through the views then we can't be sure that the offline cache is fully populated. I suppose it would be possible to have different modes of operation depending on the presence of the offline cache, but that strikes me as needlessly complex and it would roll back a lot of the performance gains that come along with the memoryCalendar cache, as we'd again have to go to the net on each getItems() call with at least an etag query.

Comment 5

11 years ago
Using the existing offline facility (and maybe improving that) sounds like the right approach to me, too. In addition to that, I even don't think we need a separate memory calendar in the caldav provider anymore, because the storage provider already caches, too (which could be improved though). One of the ideas behind the offline facility has been that the providers shouldn't implement caching, but only concentrate on implementing a specific calendar protocol.
(Assignee)

Comment 6

11 years ago
Alright then. I am going to work on this. Thanks for your suggestions.

Do you suggest the CalDAV provider should be reimplemented from scratch, in order to strip it from all its internal caching?
(Assignee)

Comment 7

11 years ago
Regarding the X-TAG thing, I would suggest instead to create an additional table in the cache database which would store metadata specific to the calendar providers. I think it would be cleaner and probably easier to handle with SQL queries.
(Assignee)

Comment 8

11 years ago
More on the idea.... 

The calCachedCalendar class could provide a "metadata" accessor where the provider could store and retrieve serialized JS objects.

The table structure would be as follow:
id: calendar id
key: dictionary key/data category (for example "etags")
subkey: data identifier (for example the href values)
value: serialized data (any object)

The key/subkey structure would help keep performance by avoiding the overhead of serialization/deserialization.

Comment 9

11 years ago
(In reply to comment #6)
> Do you suggest the CalDAV provider should be reimplemented from scratch, in
> order to strip it from all its internal caching?
Please refrain from writing something up from scratch. We need to continue on caldav sched (a goal for 0.9) and this would certainly complicate a lot.

(In reply to comment #8)
Hmm, I am quite a bit indifferent about that. I like the idea of calendar item meta data, but IMO strictly thought this is X-props, so why shouldn't we give it a try before? I see only performance considerations, because reading X-props requires reading items. But since items are already memory cached by the storage provider, this is IMO worth a try before adding a separate meta data API.
(Assignee)

Comment 10

11 years ago
Hi Daniel,

No problem regarding the "from scratch" reimplementation. I was referring to that because most of the code is meant to play with the memory calendar, which should be replaced with interractions with the storage calendar... Regarding this, we need and we plan to work on the implementation in Lightning 0.8 for our clients. Which kind of problems do you think it would pose for integrating that code in CVS? Alternatively, is it possible to simply take that CVS code into Lightning 0.8 without breaking anything. I.e., has any interface changed since that release?

Regarding the X-PROP, I understand your point and I agree that this is probably not that useful.

I am still getting used to the current code and will probably do some tests tomorrow. Is there a mean of communication where I can pose my questions during my development which would be more appropriate than this thread?
One way to think about whether or not something should have an X- property is to considered whether it needs to be round-tripped.  Export and re-import on another computer is one case; transport via iTIP is another.  

I'm not really sure what the right answer is here offhand.  In general, though, I'd prefer to err on the side of exposing something when we don't really know.

Just because something's available as an X- property doesn't mean that that X- property has to be the primary way to access it, though.  If, for some reason (eg performance), it makes sense to add a table for caching/accessing, great...
(In reply to comment #10)
> 
> I am still getting used to the current code and will probably do some tests
> tomorrow. Is there a mean of communication where I can pose my questions during
> my development which would be more appropriate than this thread?
> 

#calendar on irc.mozilla.org, or mozilla.dev.apps.calendar on news.mozilla.org are both good places.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 13

11 years ago
(In reply to comment #10)
> most of the code is meant to play with the memory calendar, which
> should be replaced with interractions with the storage calendar... 

the memory and storage calendars both implement the calICalendar interface, so this part of the change should not be too bad. It's not clear to me offhand how to get the metadata under discussion from the storage calendar into the CalDAV provider's .mItemInfoCache; that may require an interface change

> this, we need and we plan to work on the implementation in Lightning 0.8 for
> our clients. Which kind of problems do you think it would pose for integrating
> that code in CVS? Alternatively, is it possible to simply take that CVS code
> into Lightning 0.8 without breaking anything. I.e., has any interface changed
> since that release?
> 

There are some significant code changes close to landing in the CalDAV provider (bug 416239) that touch parts of the provider relevant to this discussion; more such changes (and some I think irrelevant interface changes) will be upcoming in the next month or so in relation to bug 409921. I can't speak to changes in the offline code.

(In reply to comment #11)
>One way to think about whether or not something should have an X- property is
>to considered whether it needs to be round-tripped.  Export and re-import on
>another computer is one case; transport via iTIP is another.  

In this case the only round-trip needed is between one Sunbird/Lightning session and another. But it's not clear to me that it's relevant what computer(s) those sessions run on, so I think you've got that covered. :)

Comment 14

11 years ago
why the provider load all calendar items in memory at load? it had better to load only 1 month before today and 1 month after

99% of times the user want to know what he has to do the current week (or last week, or next week)

Comment 15

11 years ago
Evolution works this way...

It retrieves first the Etag of all items with 
<C:calendar-query xmlns:C="urn:ietf:params:xml:ns:caldav" xmlns:D="DAV:">
  <D:prop>
    <D:getetag/>
  </D:prop>

It stores the etag in his database...
and fetch differences using GET's (it would be better to use multiget)

For the first import it's a little bit long (2min) with a 1000 items calendar...
but it's much better than lightning...

In lightning it seems that  the rendering time (and not the caldav request)  is the longest operation
(Assignee)

Comment 16

11 years ago
Hi Hubert,


What I am currently implementing is exactly that. The code should be ready soon, either today or tomorrow.

Comment 17

11 years ago
Great,

I'll help you to test the code/patch with our server...
(Assignee)

Comment 18

11 years ago
Created attachment 324642 [details] [diff] [review]
initial patch, needs further checks

This is an initial patch. I still need to check the provider's behaviour when adding and deleting items, both internally and externally. Otherwise the caching is functional.

Of second importance:
- added emacs tag for correct identation
- modified the declaration of mHrefIndex to be a dictionary rather than an array
- create two custom tags: X-MOZ-ETAG and X-MOZ-RESOURCEPATH
- maybe reindented some existing code

Any comments so far?
(Assignee)

Comment 19

11 years ago
Big disappointment here.... When initializing the etag and location cache (in my calDavInitialListener), the process is still very long. And it makes sense since it requires to parse all the events stored in the sqlite database. Any suggestions on how to avoid this? Can we add columns to the tables and have special methods in the storage provider to store that information instead?
(Assignee)

Comment 20

11 years ago
With a small hook, I managed to reduce the cache initialization from 1 minute to 1 second. A couple of fixes and tests and I will be ready to submit a new patch.
(Assignee)

Comment 21

11 years ago
Created attachment 324688 [details] [diff] [review]
second implementation

This version uses a different algorithm, relying on a hook in the storage calendar (following patch) to build the mHrefIndex and mItemInfoCache dictionaries. Also, it fixes the situation where all the refreshes would be queued indefinitely if no changed had happened in the etags.

I needed to remove the "-w" flag to make this patch apply correctly on trunk CVS, which will cause all my reindentation to be applied as well. Sorry for that.
Attachment #324642 - Attachment is obsolete: true
(Assignee)

Comment 22

11 years ago
Created attachment 324689 [details] [diff] [review]
the hook required in the storage provider

This patch is required for the "second implementation" to work. The diff was made from the storage provider in 0.8, but still applies correctly on trunk.
(Assignee)

Comment 23

11 years ago
Unless bugs are uncovered, those patches conclude my fix for this bug.
Assignee: nobody → WSourdeau
OS: Linux → All
Status: NEW → ASSIGNED
Wolfgang, this sounds wonderful.  Is the patch ready for review?  If so, you probably want to mark it as such in the details view for each attachment.  (You might get some pushback about the whitespace changes, early warning).

Thanks _a lot_ for this work!
Comment on attachment 324689 [details] [diff] [review]
the hook required in the storage provider

You need to provide a diff against an recent calStorageCalendar.js version. Your patch reverts many changes that were applied after the 0.8 release.
(Assignee)

Comment 26

11 years ago
(In reply to comment #24)
Thanks David!
Regarding the whitespacing, I will try to revert to something more compliant with the Lightning coding style and submit another patch tomorrow.
Regarding the reviewing, who should I put in the "requestee" field?

(Assignee)

Comment 27

11 years ago
(In reply to comment #25)
> (From update of attachment 324689 [details] [diff] [review])
> You need to provide a diff against an recent calStorageCalendar.js version.
> Your patch reverts many changes that were applied after the 0.8 release.
> 

Oops, my mistake. I was meaning to make a diff between Lightning 0.8's implementation, which applies well on trunk. I will submit another patch tomorrow morning.
re: reviewer, I dunno -- bbrowning or daniel boezle?
Wolfgang, I thought you wanted to use/enhance the existing caching infrastructure for the caldav provider? It seems you've wired a storage calendar in (removing the memory calendar). I think it makes more sense and the caldav provider slimmer to implement calIChangeLog.
About the wrappedJSObject hack getting your X-props (fetchCalDAVProps): If it turns out this is necessary, I'd rather like a clean property API added to calISyncCalendar.
Severity: critical → major
Flags: wanted-calendar0.9+

Comment 30

11 years ago
Hi, im testing yours patch, and it seems it doesn't work for me :

First Error :

#######!!!!! calStorageCalendar Schema Update failed -- db version: 9 this version: 8

Second error


flushItem DB error: not an error
flushItem DB error: not an error

....
flushItem DB error: not an error
CalDAV: refresh completed with status 207


No item appear...

Comment 31

11 years ago
Why do you downgrade the schema version from 9 to 8????
(In reply to comment #31)
Hubert, please see comment #27; sounds like the patch is not usable on the current tree and current 0.9pre profiles.
(Assignee)

Comment 33

11 years ago
Created attachment 324793 [details] [diff] [review]
new storage caldav diff against 0.8

This applies to 0.8 as well as on 0.9 without reverting anything else.
Attachment #324689 - Attachment is obsolete: true
(Assignee)

Comment 34

11 years ago
Created attachment 324812 [details] [diff] [review]
calDavCalendar.js: cleaner patch with two bugfixes

This patch has been reindented properly. The storage calendar initialization is now also done in safeRefresh, if needed. Also, the onError methd now checks if aErrNo is set before invoking toString upon it.
Attachment #324688 - Attachment is obsolete: true
(Assignee)

Comment 35

11 years ago
(In reply to comment #30)
> Hi, im testing yours patch, and it seems it doesn't work for me :
>

Hi Hubert, the latest patches should work properly.
(Assignee)

Comment 36

11 years ago
(In reply to comment #29)
Hi Daniel,

The thing is that it has to be implemented as quickly as possible with my remaining lack of knowledge regarding Lightning internals. Also I wanted to be as conservative as possible so that my clients can still run Lightning 0.8 with the least possible changes...

That being said, I would be glad to implement whatever you think is best. But since I am quite comfortable with JS and XPCom but not with Lightning's internals, I need precise directions on what needs to be implemented and with which interfaces. For example I had no knowledge of the two interfaces you mention in your comment. Also, what implication would this have with regard to the refresh and getItems methods, since those interfaces virtually renders them useless?
(Assignee)

Updated

11 years ago
Attachment #324812 - Flags: review?(daniel.boelzle)
Comment on attachment 324812 [details] [diff] [review]
calDavCalendar.js: cleaner patch with two bugfixes

Fly by comments:


>+    initStorageCalendar: function caldav_iMC() {
>+        this.mStorageCalendar =
>+        Components.classes["@mozilla.org/calendar/calendar;1?type=storage"]
>+        .createInstance(Components.interfaces.calICalendar)
>+        .wrappedJSObject;
>+        var file = getCalendarDirectory();
>+        file.append("cache-" + this.id + ".sqlite");
>+        var uri = getIOService().newFileURI(file);
>+        uri.spec += ("?id=" + this.id);
>+        this.mStorageCalendar.uri = uri;
>+        this.mStorageCalendar.id = this.id;
>+        this.mStorageCalendar.superCalendar = this;
>         this.mObserver = new calDavObserver(this);
>-        this.mMemoryCalendar.addObserver(this.mObserver);
>-        this.mMemoryCalendar.setProperty("relaxedMode", true);
>+        this.mStorageCalendar.addObserver(this.mObserver);
>+        this.mStorageCalendar.setProperty("relaxedMode", true);
It seems you are just using a separate storage calendar here, why not use calCachedCalendar?


>+                                var newItem = item.QueryInterface(Components.interfaces.calIItemBase);
>+                                newItem.setProperty("X-MOZ-ETAG", String(etag));
>+                                newItem.setProperty("X-MOZ-RESOURCEPATH", String(aResource.path));
>+
I may be wrong, but I think string conversion is done automatically on export.



>+            var query = this.mQueuedQueries.pop();
>+            this.mStorageCalendar.getItems(query[0], query[1],
>+                                           query[2], query[3],
>+                                           query[4]);
this.mStorageCalendar.getItems.apply(this.mStorageCalendar, query);

>+        var cacheItems = this.mStorageCalendar.wrappedJSObject.fetchCalDAVProperties();
This makes you depend on the storage calendar being a javascript object, using wrappedJSObject is not recommended. I think we should go a different route.

Comment 38

11 years ago
I've tested the patch...

It works like a charm for me... It reduced the loading time from 5min to 5s...
But the first loading of the calendar takes ever long time (~5 min...). Why is this first loading so long? Storing events in sqlite (do you use transaction for insert's, sqlite is very fast when using transaction)? parsing ics? i'm sure we could improve this too...

Thanks for this great work!

Comment 39

11 years ago
I think you should drop the sqlite cache file when the calendar is deleted...

Comment 40

11 years ago
It could be great if we can use sqlite transactions to do the first import which is still very long... i'm sure that it would improve performances
Comment on attachment 324793 [details] [diff] [review]
new storage caldav diff against 0.8

As already commented, I'd prefer a clean API added to calISyncCalendar.
Attachment #324793 - Flags: review-
Comment on attachment 324812 [details] [diff] [review]
calDavCalendar.js: cleaner patch with two bugfixes

Moving over review to Bruno who keeps track on caldav provider changes, and should decide which bits to take over.

In general, I think this patch (and the other one, too) serve well as a proof-of-concept, but I'd rather prefer to link the caldav provider to the existing caching/offline infrastructure (as mentioned in comment #29).
Attachment #324812 - Flags: review?(daniel.boelzle) → review?(browning)
(Assignee)

Comment 43

11 years ago
(In reply to comment #42)
Hi Daniel,


As mentionned, I would be glad to implement it in a cleaner way. I just need the time to understand the interfaces you mention and the way the cached calendar works. Since this has taken me some time already, I won't be able to do it immediately because I need to work on other things. I may have the time in a week or two though...

Updated

11 years ago
Flags: wanted-calendar0.9+ → blocking-calendar0.9+
Sounds cool, Wolfgang. If you have any questions, just mail me.
Flags: blocking-calendar0.9+ → wanted-calendar0.9+
(Assignee)

Comment 45

10 years ago
Created attachment 331045 [details] [diff] [review]
new implementation

New combined patch with all the changes required.
Attachment #324793 - Attachment is obsolete: true
Attachment #324812 - Attachment is obsolete: true
Attachment #324812 - Flags: review?(browning)
(Assignee)

Comment 46

10 years ago
The above patch is a combination of all the diff I had to make to calDavCalendar.js, calStorageCalendar.js and calIChangeLog.idl.
At least 2 things are still needed:
- fixing the situation causing a deleted event would be kept displayed
- apply the new methods in calIChangeLog to calMemoryCalendar

However, it should show the philosophy behind my new implementation. Please review it and send me your comments so that I can produce a final patch as soon as possible.
(Assignee)

Updated

10 years ago
Attachment #331045 - Flags: review?(daniel.boelzle)
(Assignee)

Comment 47

10 years ago
Created attachment 332097 [details] [diff] [review]
suggested final patch

Hi,

This is my latest implementation.
I have found a much more simple way of gluing things, avoiding some bugs in the meantime. I suggest this patch, which applies cleanly to current CVS, under the calendar/ directory, as a final implementation.
Attachment #331045 - Attachment is obsolete: true
Attachment #332097 - Flags: review?(daniel.boelzle)
Attachment #331045 - Flags: review?(daniel.boelzle)
Comment on attachment 332097 [details] [diff] [review]
suggested final patch

>+[scriptable, uuid(ca4fa73e-58ff-11dd-968e-001f5be86cea)]
>+interface calISyncMetaData : nsISupports {
>+  readonly attribute AUTF8String key;
>+  readonly attribute AUTF8String value;
>+};

That looks a whole lot like nsIProperty...

> [scriptable, uuid(cef78f0e-bdbe-4534-8546-29b3e85be148)]
> interface calISyncCalendar : calICalendar {
>+  void storeMetaData(in AUTF8String key, in AUTF8String value);
>+  void deleteMetaData(in AUTF8String key);
>+  AUTF8String fetchMetaData(in AUTF8String key);
>+  void fetchAllMetaData(out PRUint32 count,
>+			[array, size_is(count), retval] out calISyncMetaData aSyncItemVersionInfo);
> };

And that's similar to nsIWriteablePropertyBag2.

Maybe use those two interfaces? You still can use your own implementation.
Anyway, please add some comments describing what kind of meta data this is about. How is it different from the preferences?

>+  cal_metadata:
>+    "	cal_id		INTEGER, " +
>+    "   meta_key	TEXT," + 
>+    "   meta_value	TEXT" + 
>+    ""

This changes the schema. Shouldn't there be migration code, and a version increase?

And I have a small nit: there are tabs in the patch...

Updated

10 years ago
Depends on: 449031
(In reply to comment #48)
> (From update of attachment 332097 [details] [diff] [review])
> >+[scriptable, uuid(ca4fa73e-58ff-11dd-968e-001f5be86cea)]
> >+interface calISyncMetaData : nsISupports {
> >+  readonly attribute AUTF8String key;
> >+  readonly attribute AUTF8String value;
> >+};
> 
> That looks a whole lot like nsIProperty...

Yes, moreover I don't see really a need for an extra interface here, we could return two arrays (ids, values) of the same length.

> > [scriptable, uuid(cef78f0e-bdbe-4534-8546-29b3e85be148)]
> > interface calISyncCalendar : calICalendar {
> >+  void storeMetaData(in AUTF8String key, in AUTF8String value);
> >+  void deleteMetaData(in AUTF8String key);
> >+  AUTF8String fetchMetaData(in AUTF8String key);
> >+  void fetchAllMetaData(out PRUint32 count,
> >+			[array, size_is(count), retval] out calISyncMetaData aSyncItemVersionInfo);
> > };
> 
> And that's similar to nsIWriteablePropertyBag2.

The problem is that XPCOM doesn't allow multiple inheritance (AFAIK) and composition is IMO not correct here.

> >+  cal_metadata:
> >+    "	cal_id		INTEGER, " +
> >+    "   meta_key	TEXT," + 
> >+    "   meta_value	TEXT" + 
> >+    ""
> 
> This changes the schema. Shouldn't there be migration code, and a version
> increase?
right

> And I have a small nit: there are tabs in the patch...
right

I've separated out the meta data code into bug 449031: added documentation, untabified and changed it, removed leftovers and flaws, added unit tests... so we can concentrate on the caldav changes in this bug.
(Assignee)

Comment 50

10 years ago
Created attachment 332226 [details] [diff] [review]
revised final patch

Hi,


Thanks for reviewing my previous patch.
The new attachment is an adapted version of calDavCalendar.js alone.
A few changes related and unrelated to the above comments:
- added myself in the list of contributors
- fixed fetchItemVersions to use Daniel's suggested api (in #449031)
- removed references to "retVal", which is never used
- removed empty lines
- commented out the hack refreshing other cals in the same HTTP realms for 2 reasons:
  - I think this should be handled by the login dialogs, or the code invoking them
  - it causes problems related to the calIChangeLog API
- in getCalendarData, a bug in SOGo revealed a limitation of the code: sometimes SOGo returns a 200 code for getetag while returning 404 for the corresponding calendar-data. Fetching the status in this situation and calling split(" ") would generate an exception ("split is not a function"). What I changed is that I take all the return codes into account. Whenever a non-200 happens, the process is handled accordingly.
Attachment #332097 - Attachment is obsolete: true
Attachment #332226 - Flags: review?(daniel.boelzle)
Attachment #332097 - Flags: review?(daniel.boelzle)
(Assignee)

Comment 51

10 years ago
.... and I added 3 more tabs, at line 169 to 171. Sorry for that...

Updated

10 years ago
Flags: tb-integration+
(Assignee)

Updated

10 years ago
Depends on: 449401
No longer depends on: 449031
(Assignee)

Comment 52

10 years ago
Created attachment 332943 [details] [diff] [review]
final patch

This patch has the indentation fixed, adapations to the latest CVS commits, and manages the few error cases that were missing. I consider it final.
Attachment #332226 - Attachment is obsolete: true
Attachment #332943 - Flags: review?(browning)
Attachment #332226 - Flags: review?(daniel.boelzle)
Comment on attachment 332943 [details] [diff] [review]
final patch

>+    resetLog: function caldav_rL() {
>+//         LOG("reset Log");
>+    },
Shouldn't a call to resetLog() reset the metadata?

>+    replayChangesOn: function caldav_rCO(aDestination, aListener) {
>+        this.mChangeLogListener = aListener;
>+        if (!this.mTargetCalendar) {
>+            this.mTargetCalendar = aDestination.wrappedJSObject;
Do you really need wrappedJSObject here? What implementation specific things do you use?

>+        }
>+        else
>+            this.safeRefresh();
Fix the else as in style guide, \n } else { \n ... \n }. Happens here and elsewhere


>+            var itemDataArray = itemData.split("#");
>+            var etag = itemDataArray[0];
>+            var resourcePath = itemDataArray[1];
Can the resource path or etag possibly contain an # ? If the database stores this correctly, you could use the unicode substitution character \u001A

>     get canRefresh() {
>-        return true;
>+        return !this.isCached;
>     },
cached calendars can't refresh? Please explain.

>+//                 if (thisCalendar.mAuthScheme == "Digest" &&
>+//                     thisCalendar.firstInRealm()) {
>+//                     thisCalendar.refreshOtherCals();
>+//                 }
IIRC Bruno said we need this?


>+                    if (thisCalendar.isCached)
>+                        thisCalendar.mChangeLogListener.onResult(null,
>+                                                                 Components.results.NS_ERROR_FAILURE);
I believe you need  the status in the operation, i.e

...onResult({ status: Components.results.NS_ERROR_FAILURE }, null);

Happens here and in some more places.

Also, why is the changelog listener stored on the calendar? I think it would be better to use it directly from the passed argument.

>+                  var itemStatus = "" + response..D::["status"].*[k];
>+                  var status = itemStatus.split(" ")[1];
>+                  if (status != 200) {
>+                      hasNon200 = hasNon200 || true;
>+                      if (status == 404)
>+                          has404 = has404 || true;
Doesn't this always evaluate to true, regardless of what has404 was previously? => |has404 = true;|
>+                      if (non200Statuses.indexOf(status) < 0)
>+                          non200Statuses.push(status);
Brackets even for one-line if's here and in other places.


Code-level review only, leaving the rest for Bruno
(Assignee)

Comment 54

10 years ago
(In reply to comment #53)
> (From update of attachment 332943 [details] [diff] [review])
> >+    resetLog: function caldav_rL() {
> >+//         LOG("reset Log");
> >+    },
> Shouldn't a call to resetLog() reset the metadata?

It could be done but I think it's useless considering that it's called when creating a new calendar and that each calendar has its own cache now.

> >+    replayChangesOn: function caldav_rCO(aDestination, aListener) {
> >+        this.mChangeLogListener = aListener;
> >+        if (!this.mTargetCalendar) {
> >+            this.mTargetCalendar = aDestination.wrappedJSObject;
> Do you really need wrappedJSObject here? What implementation specific things do
> you use?

This is no longer needed. I was using it because I use the nightly XPI as development base and the required API were not yet available.

> 
> >+        }
> >+        else
> >+            this.safeRefresh();
> Fix the else as in style guide, \n } else { \n ... \n }. Happens here and
> elsewhere

OK

> >+            var itemDataArray = itemData.split("#");
> >+            var etag = itemDataArray[0];
> >+            var resourcePath = itemDataArray[1];
> Can the resource path or etag possibly contain an # ? If the database stores
> this correctly, you could use the unicode substitution character \u001A

How do I do that?

> cached calendars can't refresh? Please explain.

cached calDav calendars cannot refresh at first because the storage calendar is not yet provided when refresh() is first called. Furthermore, it's the replayChangesOn which really update the cached items, so we can let the refresh be accomplished by the calCachedCalendar instead and not worry about this.

> 
> >+//                 if (thisCalendar.mAuthScheme == "Digest" &&
> >+//                     thisCalendar.firstInRealm()) {
> >+//                     thisCalendar.refreshOtherCals();
> >+//                 }
> IIRC Bruno said we need this?

Yes and no, this is left to debate because I think this should not be put there. It's a hack meant to avoid different popup dialogs for the same realm on the same server. Instead of doing this, the authentication part of necko should be fixed, if we want to be clean. That is also why I commented this out.


I'll answer the rest a bit later.
(In reply to comment #54)
> (In reply to comment #53)
> > (From update of attachment 332943 [details] [diff] [review] [details])
> > >+    resetLog: function caldav_rL() {
> > >+//         LOG("reset Log");
> > >+    },
> > Shouldn't a call to resetLog() reset the metadata?
> 
> It could be done but I think it's useless considering that it's called when
> creating a new calendar and that each calendar has its own cache now.
Unless its a lot of work, I think it would be nice. I have an extension that allows to manually do some things, and resetting the changelog is one of those things.


> > >+            var itemDataArray = itemData.split("#");
> > >+            var etag = itemDataArray[0];
> > >+            var resourcePath = itemDataArray[1];
> > Can the resource path or etag possibly contain an # ? If the database stores
> > this correctly, you could use the unicode substitution character \u001A
> 
> How do I do that?
Quite easy, just replace "#" with "\u001A". In the above code, and in setMetaData, and wherever else you set itemData: Example:

var itemDataArray = itemData.split("\u001A");

(Assignee)

Comment 56

10 years ago
(In reply to comment #55)
> (In reply to comment #54)
> > (In reply to comment #53)
> > > (From update of attachment 332943 [details] [diff] [review] [details] [details])
> > > >+    resetLog: function caldav_rL() {
> > > >+//         LOG("reset Log");
> > > >+    },
> > > Shouldn't a call to resetLog() reset the metadata?
> > 
> > It could be done but I think it's useless considering that it's called when
> > creating a new calendar and that each calendar has its own cache now.
> Unless its a lot of work, I think it would be nice. I have an extension that
> allows to manually do some things, and resetting the changelog is one of those
> things.

I have implemented it this way in my local copy:

    resetLog: function caldav_rL() {
        if (this.isCached && this.mTargetCalendar) {
            this.mTargetCalendar.prepareBatchUpdate();
            for (var itemId in this.mItemInfoCache) {
                this.mTargetCalendar.deleteMetaData(itemId);
                delete this.mItemInfoCache[itemId];
            }
            this.mTargetCalendar.finishBatchUpdate();
        }
    },

(The batch update methods are future methods to be submitted while solving https://bugzilla.mozilla.org/show_bug.cgi?id=450104)
 
> Quite easy, just replace "#" with "\u001A". In the above code, and in
> setMetaData, and wherever else you set itemData: Example:
> 
> var itemDataArray = itemData.split("\u001A");

Done! 

(Assignee)

Comment 57

10 years ago
> >+                    if (thisCalendar.isCached)
> >+                        thisCalendar.mChangeLogListener.onResult(null,
> >+                                                                 Components.results.NS_ERROR_FAILURE);
> I believe you need  the status in the operation, i.e
> 
> ...onResult({ status: Components.results.NS_ERROR_FAILURE }, null);
> 
> Happens here and in some more places.

Fixed (locally), thanks!

> Also, why is the changelog listener stored on the calendar? I think it would be
> better to use it directly from the passed argument.

Fixed

> 
> >+                  var itemStatus = "" + response..D::["status"].*[k];
> >+                  var status = itemStatus.split(" ")[1];
> >+                  if (status != 200) {
> >+                      hasNon200 = hasNon200 || true;
> >+                      if (status == 404)
> >+                          has404 = has404 || true;
> Doesn't this always evaluate to true, regardless of what has404 was previously?

Yes. Anyway this has been obsoleted, so I removed it altogether.

> => |has404 = true;|
> >+                      if (non200Statuses.indexOf(status) < 0)
> >+                          non200Statuses.push(status);
> Brackets even for one-line if's here and in other places.

Fixed.


All those changes in my next upload...
(Assignee)

Updated

10 years ago
Depends on: 450104
No longer depends on: 449401
(Assignee)

Comment 58

10 years ago
Created attachment 334003 [details] [diff] [review]
final patch

This patch has all the comments above taken into account. It has also been readapted to the latest code in CVS.

However, please note it also has API changes that will be provided with the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=450104.
Attachment #332943 - Attachment is obsolete: true
Attachment #334003 - Flags: review?(philipp)
Attachment #332943 - Flags: review?(browning)
It would be really great to have this in 0.9
Flags: blocking-calendar0.9?
This won't block 0.9, but is of course welcome if landing in time.
Flags: blocking-calendar0.9? → blocking-calendar0.9-
Comment on attachment 334003 [details] [diff] [review]
final patch

>+//     this.mAuthScheme = null;
>+//     this.mAuthRealm = null;
We should either clarify if we still need this and remove all related comments, or keep it in and fix the problem that arises from it.



>+            this.fetchItemVersions();
>+            this.checkDavResourceType(aChangeLogListener);
>+        }
>+        else {
} else {


>+            var itemDataArray = itemData.split("\u001A");
>+            var etag = itemDataArray[0];
>+            var resourcePath = itemDataArray[1];
You should be able to do:
var [etag, resourcePath] = itemData.split("\u001A");

>+              this.mTargetCalendar.setMetaData(id, dataString);
>+            }
>+            else
>+              LOG("CAlDAV: cannot store meta data without an id");
>+        }
>+        else
>+            LOG("CalDAV: calendar storage does not support meta data");
>+    },

} else { and use brackets also for one-line else blocks.


>+            if (thisCalendar.isCached) {
>+//                 LOG("expected to end batch update");
>+                thisCalendar.mTargetCalendar.finishBatchUpdate();
>+//                 LOG("expected to have ended batch update");
Remove comments

>+        LOG("freebusy...");
Stray comment, either elaborate and prefix with CalDAV, or remove.


r=philipp with comments fixed.
Attachment #334003 - Flags: review?(philipp) → review+

Comment 62

10 years ago
Let's hold off on committing this: I really want to give it a good review and expect to be able to do so this evening. The patch will have bitrotted considerably with the addition of the new caldav-sched code, which will also make us want to cache an extra piece of metadata (.isInBoxItem). And so forth. I think it's premature to commit.
Attachment #334003 - Flags: review?(browning)
(Assignee)

Comment 63

10 years ago
I will probably have other fixes as well, especially since the changelog listener is now passed as parameter. I am actually awaiting your final commit for the scheduling code to rereredo my patch.
We're heading for 0.9 RC1 the next days. Would be nice get more traction on this, please. Wolfgang, if you've an updated patch, please submit it.

Comment 65

10 years ago
A couple of thing I've noticed that you'll want to deal with as you de-bitrot this patch:
1) as we discussed on irc, we really do need to keep the whole authscheme/firstInRealm/etc bits in there. Don't want to break Apple for 0.9, and making anyone who has more than one calendar re-enter credentials every 15 minutes does that.
2) the cached metadata will now need to include isInBoxItem as well, for caldav-sched support (ironically enough, it's needed for all servers except SOGo)
Wolfgang, can we expect a new patch for this, *today*? We are spinning rc1 this week and need to checkin/review things today so that ause can prepare the needed steps with tomorrow's nightlies.
(Assignee)

Comment 67

10 years ago
Created attachment 337534 [details] [diff] [review]
final patch

Final patch, rerereadapted to the latest HEAD code, and with the above comments taken into account.
Attachment #334003 - Attachment is obsolete: true
Attachment #337534 - Flags: review?
Attachment #334003 - Flags: review?(browning)
(Assignee)

Updated

10 years ago
Attachment #337534 - Flags: review? → review?(philipp)
Created attachment 337544 [details] [diff] [review]
[checked in] Review Comment patch

This patch contains my review comments, r=philipp on this patch.

Due to the rc tomorrow I'm checking this in now, please note the change in the storage calendar was neccessary since obviously setMetaData is used with something other than a string/int/whatever. This needs investigation!

Bruno, please do take a look at this patch for a further review.
Attachment #337534 - Attachment is obsolete: true
Attachment #337544 - Flags: review+
Attachment #337534 - Flags: review?(philipp)
Checked in on HEAD and MOZILLA_1_8_BRANCH and SUNBIRD_0_9_BRANCH

-> Keeping open for now
Target Milestone: --- → 0.9
Attachment #337544 - Flags: review?(browning)
Comment on attachment 337544 [details] [diff] [review]
[checked in] Review Comment patch

>             var ctag = multistatus..CS::getctag;
>             if (!ctag || ctag != thisCalendar.mCtag) {
>                 // ctag mismatch, need to fetch calendar-data
>                 thisCalendar.mCtag = ctag;
>+                thisCalendar.mTargetCalendar.setMetaData("ctag", ctag);
I can imagine an explicit ctag.toString() is missing here. Same applies for the etag.

Updated

10 years ago
Attachment #337544 - Flags: review?(browning) → review-

Comment 71

10 years ago
Comment on attachment 337544 [details] [diff] [review]
[checked in] Review Comment patch

Before looking at the code, I smoketested a fresh build with this patch against DAViCal, Bedework, Apple, and SOGo, and ran across some issues that I think will need to be dealt with before we release 0.9 with this code, including
* CalDAV calendars which do not have cache enabled throw a MODIFICATION_FAILED error on deleteItem(), which effectively means that all CalDAV calendars will need to use the (experimental!) cache. My preference would be to allow use of CalDAV calendars without cache, but if it is going to be required we should force that, including both creating the cache and making the option in the calendar-properties UI checked-but-disabled. Failing to do that will at the very least confuse every CalDAV user we have.
* regardless of the state of the calendar.caldav.sched.enabled preference, this code LOGs that the calendar does not support CalDAV scheduling, and use of caldav-sched is in fact not possible, including freebusy - which is working in 0.8. I think the loss of freebusy would be an unacceptable regression, and would strongly prefer that we have a functional calendar.caldav.sched.enabled pref in hopes of getting more CalDAV testing during the 0.9 cycle.
So ... I'm thinking this patch needs a spot more work before we can ship it.
(Assignee)

Comment 72

10 years ago
I have noticed another "bug", introduced in my patch. At around line 1244, the sequence:

                if (!item) {
                    this.notifyOperationComplete(aListener,

should be replaced with:

                if (!item) {
                    thisCalendar.notifyOperationComplete(aListener,

I'll answer to Bruno's comments tomorrow morning, after a good night.
Created attachment 337693 [details] [diff] [review]
Overhaul - v1

Sorry for so many changes, but when debugging and fixing I went amok and needed to change some things. I found a couple of bugs on the way.

r=dbo over the shoulder earlier today, please do take an extra look at this!
Attachment #337693 - Flags: review+
Created attachment 337697 [details] [diff] [review]
[checked in] Overhaul - v1

Oops, old patch
Attachment #337693 - Attachment is obsolete: true
Attachment #337697 - Flags: review+
Attachment #337697 - Attachment description: Overhaul - v1 → [checked in] Overhaul - v1
Attachment #337544 - Attachment description: Review Comment patch → [checked in] Review Comment patch
I've checked this in to move things forward, this bug can be closed tomorrow after the nightlies have been spun.
Closing now, please open new bugs for regressions

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 77

10 years ago
What I'm loving in this closed bug is that normal users have a sunbird which start in 2 minutes 50 to open 10 caldav agenda which have 3160 events in them (overall events).

If you think this is normal, I applause the non quality.

Did we use the (evil experimental cache ? ) or not ... 
This question was not answered.
Bruno: this bug was tracking a specific sort of optimization, and it's closed because that particular code was checked in.  Given the numbers you quote, there is clearly more performance work to do.  If you would a new bug with more specific numbers (eg machine speed, memory, as well as the ones you quote above), that would be helpful.
You need to log in before you can comment on or make changes to this bug.