Last Comment Bug 529326 - Create indexes for the local calendar cache
: Create indexes for the local calendar cache
Status: RESOLVED FIXED
[needed beta][no l10n impact]
: perf, relnote
Product: Calendar
Classification: Client Software
Component: Provider: Local Storage (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: 1.0b1
Assigned To: Philipp Kewisch [:Fallen]
:
Mentors:
Depends on:
Blocks: 498968
  Show dependency treegraph
 
Reported: 2009-11-17 11:14 PST by Simon Vaillancourt
Modified: 2011-11-07 04:01 PST (History)
0 users
philipp: blocking‑calendar1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix - v1 (5.76 KB, patch)
2009-11-23 01:28 PST, Philipp Kewisch [:Fallen]
no flags Details | Diff | Splinter Review
Fix - v2 (10.69 KB, patch)
2009-11-23 04:25 PST, Philipp Kewisch [:Fallen]
mschroeder: review+
Details | Diff | Splinter Review

Description Simon Vaillancourt 2009-11-17 11:14:53 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.15) Gecko/2009101601 Firefox/2.0.0.18 X-ORACLE-DEBUG=STATS (.NET CLR 3.5.30729)
Build Identifier: 

Using the cache for many calendars can make Thunderbird startup time really slow and cpu intensive, I've investigated this with a 0.9 build and found that simply adding indexes would help a lot, one user went from a 45 seconds startup time to a 7 seconds startup time. I added the indexes in initDB of calStorageCalendar.js , they were done based on the statements i saw in that file so they might now all yield the same speed improvement, creating events has shown no noticeable slowdowns. I currently do not have time to provide a proper patch for 1.0 so I'm dumping the indexes i added in 0.9 here:

           this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_properties_cal_id ON cal_properties(cal_id);"
                );        
            this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_event_cal_id ON cal_events(cal_id);"
                );
            this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_event_item_id ON cal_events(cal_id, id);"
                );
            this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_attendees_item_id ON cal_attendees(cal_id,item_id);"
                );
            this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_recurrence_item_id ON cal_recurrence(cal_id,item_id);"
                );
            this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_metadata_item_id ON cal_metadata(cal_id,item_id);"
                );
            this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_metadata_cal_id ON cal_metadata(cal_id);"
                );
            this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_attendees_recurrence_id ON cal_attendees(cal_id,item_id,recurrence_id);"
                );            
            this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_properties_recurrence_id ON cal_properties(cal_id,item_id,recurrence_id);"
                );
            this.mDB.executeSimpleSQL(
                "CREATE INDEX IF NOT EXISTS " + 
                "idx_cal_events_recurrence_id ON cal_events(cal_id,id,recurrence_id);"
                );               

Reproducible: Always
Comment 1 Philipp Kewisch [:Fallen] 2009-11-17 12:44:36 PST
Yeah, we need these. I've always procrastinated on this, thinking we could just do this with the storage2 improvements, but maybe we should do this sooner.

Even though this could well be done outside of the updater, I think we should spend a dedicated storage db version for this type of upgrade.
Comment 2 Philipp Kewisch [:Fallen] 2009-11-23 01:28:08 PST
Created attachment 414007 [details] [diff] [review]
Fix - v1

This patch takes care. I've used a slightly different set of indexes, based on our SELECT queries. There is still room for optimization, we could take a closer look at our queries with EXPLAIN. Especially the long ranged queries from cal_events and cal_todos might be optimizable when adding indexes to event_start/event_end, but I'd like to take a closer look at that some other time, since its not crystal clear which fields to index over best.
Comment 3 Philipp Kewisch [:Fallen] 2009-11-23 01:45:20 PST
Comment on attachment 414007 [details] [diff] [review]
Fix - v1

Argh, aside from forgetting to remove creating the old index, this patch doesn't create indexes on new profiles.

Back to the drawing board. I'd still appreciate if you could take a look to see if I'm going in the right direction.
Comment 4 Philipp Kewisch [:Fallen] 2009-11-23 04:25:53 PST
Created attachment 414027 [details] [diff] [review]
Fix - v2

This takes care. I've re-used the table-data object to contain the index data too. Just relying on the prefix of the table name (i.e "idx_") might not be the best solution, but it fits in well with the current implementation, and its quite unlikely that someone will name his table idx_*. I've also documented this info.
Comment 5 Philipp Kewisch [:Fallen] 2009-11-23 04:28:58 PST
One downside may be that someone upgrading between 0.9 and 1.0pre with a lot of events might expierence a one-time slow startup. We should either relnote this or add some debug output like "Indexing calendar data. This may take a while, but will not happen again". The latter won't be localized though, and it might not be worth it either.
Comment 6 Martin Schröder [:mschroeder] 2009-11-23 16:09:34 PST
Comment on attachment 414027 [details] [diff] [review]
Fix - v2

>diff --git a/calendar/lightning/install.rdf b/calendar/lightning/install.rdf
>--- a/calendar/lightning/install.rdf
>+++ b/calendar/lightning/install.rdf
[...]

Changing this file seems to be wrong for this patch!


>diff --git a/calendar/providers/storage/calStorageCalendar.js b/calendar/providers/storage/calStorageCalendar.js
>--- a/calendar/providers/storage/calStorageCalendar.js
>+++ b/calendar/providers/storage/calStorageCalendar.js
[...]

We should restrict the SELECT at http://mxr.mozilla.org/comm-central/source/calendar/providers/storage/calStorageCalendar.js#926 to |icalString| to opitmize the query.


>diff --git a/calendar/providers/storage/calStorageUpgrade.jsm b/calendar/providers/storage/calStorageUpgrade.jsm
>--- a/calendar/providers/storage/calStorageUpgrade.jsm
>+++ b/calendar/providers/storage/calStorageUpgrade.jsm
[...]

>+/**
>+ * Helper function to create an index on the database if it doesn't already
>+ * exist.
>+ *
>+ * @param db            The database to create the index on.
>+ * @param idxName       The index name.
>+ * @param idxOn         The part after the ON in CREATE INDEX ... ON.
>+ */
>+function createIndex(tblData, tblName, colNameArray, db) {

The |@param|s don't match the arguments of the function!

>+        // Alarms, Attachments, Attendees, Relations
>+        for each (let tblName in ["alarms", "attachments", "attendees", "relations"]) {
>+            createIndex(tbl, "cal_" + tblName, ["cal_id", "item_id", "recurrence_id"], db);
>+            createIndex(tbl, "cal_" + tblName, ["cal_id",
>+                                           "item_id",
>+                                           "recurrence_id",
>+                                           "recurrence_id_tz"], db);
>+        }

The second index should be enough because it contains the first one (after reading http://www.sqlite.org/optoverview.html). Please, also fix the alignment!

>+        // Metadata
>+        createIndex(tbl, "cal_metadata", ["cal_id"], db);
>+        createIndex(tbl, "cal_metadata", ["item_id", "cal_id"], db);

After changing the order in WHERE for http://mxr.mozilla.org/comm-central/source/calendar/providers/storage/calStorageCalendar.js#909, we can use |createIndex(tbl, "cal_metadata", ["cal_id", "item_id"], db);| and need only one index.

>+        // Properties. To align naming, we first delete the old index here.
>+        executeSimpleSQL(db, "DROP INDEX IF EXISTS idx_cal_properies_item_id");
>+        createIndex(tbl, "cal_properties", ["cal_id", "item_id"], db);
>+        createIndex(tbl, "cal_properties", ["cal_id",
>+                                           "item_id",
>+                                           "recurrence_id",
>+                                           "recurrence_id_tz"], db);

The second index should be enough because it contains the first one. Please, also fix the alignment!

r=mschroeder... without much testing (maybe I make up for it tomorrow), but we need it for beta (where we could get the best testing ;) ).
Comment 7 Philipp Kewisch [:Fallen] 2009-11-24 01:12:18 PST
All comments taken care of and did another round of testing.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/bb7e22c8d601>
and comm-1.9.1 <http://hg.mozilla.org/releases/comm-1.9.1/rev/b6886ae54d66>

-> FIXED
Comment 8 Philipp Kewisch [:Fallen] 2011-11-07 04:01:07 PST
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".

Note You need to log in before you can comment on or make changes to this bug.