Profiling data that I've seen points to spending the vast majority of our time during a getItems call in the if (flags & CAL_ITEM_FLAG_HAS_PROPERTIES) { line. Rather than filling up the entire propertybag at once, we ought to make it so that storage can dynamically look up those properties after they are requested.

I'm sticking this here for safe keeping, and because if I keep banging away at the subtleties of makeMemberAttr and friends, I think my head might explode. This patch works great for single events, but because recurrenceId counts as a property, I'm running into big problems there. Note that in order to see actual performance improvement you also need a small patch to calItemBase.js.

I *think* this does the trick. Since both mvl's and my data pointed to the item-creation being the slow part, this takes the biggest chunk of that and postpones it until it is needed. Basic testing shows full functionality is kept. I'll get more rigorous testing, as well as startup/view-switching numbers later. (or if someone wants to do that for me, great).

Testing results using the calendar in bug 313265: (getItems, occurrences, for October) Without patch: ~2600ms With patch: ~2000ms Note that the full 30% improvement won't quite make its way into the view, as the getProperty("CATEGORIES") call will now take slightly longer. mvl's indices will help this. I'd expect about 25% improvement in view time overall.

This patch fixes one minor bug in the previous patch, and then blows the perf wins of that one out of the water by actually using mItemCache properly in this calendar. With this patch, the previous call is now right around 1000ms. That's right < 1ms/item. For those scoring at home, the long-pole (~25%) is now getOccurrences(), which is probably about as fast as it can be (it's even in c++!)

(In reply to comment #4) > With this patch, the previous call is now right around 1000ms. That's right < > 1ms/item. For those scoring at home, the long-pole (~25%) is now > getOccurrences(), which is probably about as fast as it can be (it's even in > c++!) > Correction: getOccurrences is 90% of our time, the other 75% that i thought we were spending elsewhere was actually spent in the callback (the views).

Comment on attachment 243557 [details] [diff] [review] go speed racer >Index: calendar/providers/storage/calStorageCalendar.js >=================================================================== >+function setDateTime(aNativeTime, aTimezone, aDateTime) { >+ aDateTime.nativeTime = aNativeTime; >+ if (aTimezone && aTimezone != "floating") { >+ aDateTime.getInTimezone(aTimezone); >+ } else { >+ aDateTime.timezone = "floating"; >+ } >+} You need to assign aDateTime.getInTimezone(aTimezone) to something. >- //dump ("++++ getItems took: " + (profEndTime - profStartTime) + " ms\n"); >+ dump ("++++ getItems took: " + (profEndTime - profStartTime) + " ms\n"); Comment this back out. >+ this.mSelectPropertyForItem = createStatement( >+ this.mDBTwo, >+ "SELECT * FROM cal_properties " + >+ "WHERE item_id = :item_id AND recurrence_id = :recurrence_id " + >+ "IS NULL AND key = :key" >+ ); This query has an error. It should be ..."recurrence_id IS NULL AND key = :key" >- getAdditionalDataForItem: function (item, flags) { >+ getAdditionalDataForItem: function stor_getAddtnalData(item, flags) { This function name is strange. Either add the 3 letters to spell out "additional", or use "Addl" >+ //XXX does this actually get it right, probably not Lose this comment. >+ selectItem = storCal.mSelectPropertiesForItemWithRecurrenceId; >+ storCal.setDateParamHelper(selectItem.params, "recurrence_id", item.recurrenceId); >+ } >+ > selectItem.params.item_id = item.id; > > while (selectItem.step()) { >+ row = selectItem.row; >+ propBag.setProperty (row.key, row.value); >+ } >+ selectItem.reset(); >+ >+ // Don't forget the old props >+ var e = queryHelper.oldProps.enumerator; >+ while (e.hasMoreElements()) { >+ var prop = e.getNext().QueryInterface(Components.interfaces.nsIProperty); >+ propBag.setProperty(prop.name, prop.value); >+ } > >- selectItem.params.item_id = item.id; >- There's some crazy indenting going on in there that should be fixed and trailing spaces to be removed. >+ wrapper.setProperty = function (aName, aVal) { >+ return wrapper.__proto__.setProperty(aName, aVal); >+ }; Is this anonymous function okay or should it be renamed? >Index: calendar/base/src/calItemBase.js >=================================================================== >- var e = this.mProperties.enumerator; >- while (e.hasMoreElements()) { >- var prop = e.getNext().QueryInterface(Components.interfaces.nsIProperty); >- var val = prop.value; >- >- if (prop.value instanceof Components.interfaces.calIDateTime) { >- if (prop.value.isMutable) >- prop.value.makeImmutable(); >- } >- } >- Please file a follow up to fix any adverse affects this has on the other providers. Those are all just nits. Nice job on the perf win. r=lilmatt

Comment on attachment 243557 [details] [diff] [review] go speed racer stealing this review request.

Let's first add indexes before going for much more invasive changes like this. I fear that this patch might speed up some operations (viewing) but might slow down others (opening event dialog)

(In reply to comment #8) Two comments: 1.) If we're not going to take this whole patch right away, we should factor out the mItemCache part. That's a *really* big win requiring only small changes. It also has no adverse perf costs. 2.) Yes, this patch will make opening the event dialog, and serializing an event from a storage calendar to disk take longer. However, given that those are much less common (maybe 10 per session) than item-creation (100's per getItems query in some cases), I think that's definitely a tradeoff we should accept.

Comment on attachment 243557 [details] [diff] [review] go speed racer >Index: calendar/providers/storage/calStorageCalendar.js >+ var wrapper = item.wrappedJSObject; >+ wrapper.__defineGetter__("mProperties", function() { This is very wrong. It break all kinds of xpcom rules. In using xpcom, you can't assume anything about the implementation of the object. With this patch, you assume the implementation is in JavaScript, and is really our implementation. You can't do that. (We still need to do benchmarks to see if we really want this approach, this review comment is not an implicit agreement)

Re-assigning my bugs to nobody@mozilla.org due to recent developments.

Since we have a recurring item cache and are heading towards similar improvements like putting the whole ICS into one blob (bug 498968) or initializing the item instances lazily (bug 685884), I think we can close this bug.