This is most easily seen when forcing a refresh of a large ics calendar. The fix for Bug 710351 improves this for cached calendars, I think we can take that approach further and apply it to memory calendars and calFilter.

Severity: Critical, because this causes continuous (!) freezes of all of Thunderbird (definition of "Critical"), which makes Thunderbird unusable. I didn't know the cause, and I was close to dropping Thunderbird. Sadly, many people see this, see bug 441710, and they won't know the cause.

Comment on attachment 714743 [details] [diff] [review] Part 1: memory provider The only potential problem I see is that we have some code floating around that assumes that either storage or memory or both calendar types are actually synchronous. I've checked a few locations that use the memory calendar and I couldn't find anything, I just wanted to make sure you are aware of this. Did you check the actual performance win on this, maybe with the gecko profiler?

> Did you check the actual performance win on this Note that this is not about being faster, but about blocking, see comment 2.

(In reply to Philipp Kewisch [:Fallen] from comment #3) > Comment on attachment 714743 [details] [diff] [review] > Part 1: memory provider > > The only potential problem I see is that we have some code floating around > that assumes that either storage or memory or both calendar types are > actually synchronous. > > I've checked a few locations that use the memory calendar and I couldn't > find anything, I just wanted to make sure you are aware of this. If getItems calls on the memeory provider already call into cal.postPone, which in turn does a dispatch on the current thread, then these changes shouldn't really affect the calling code in terms of expecting them to be synchronous, right? > Did you check the actual performance win on this, maybe with the gecko > profiler? There may be a slight perf cost here, but it should be minimal and I think the responsiveness gains outweigh it.

Comment on attachment 833396 [details] [diff] [review] Part 1: memory provider v2 Matthew, please do not combine code style changes with substantial changes. That makes patches very hard to read. Separate them into 2 patches, in fact this doesn't even belong in this bug. You can fix code style, if it affects the line that you change anyways. Please leave other code parts unchanged.

Comment on attachment 833396 [details] [diff] [review] Part 1: memory provider v2 This patch changes a "for (e in a)" into "cal.forEach(a, e => {})". cal.forEach() dispatches the whole iteration to a new thread: http://mxr.mozilla.org/comm-central/source/calendar/base/modules/calIteratorUtils.jsm#51

(In reply to Ben Bucksch (:BenB) from comment #7) > Comment on attachment 833396 [details] [diff] [review] > Part 1: memory provider v2 > > Matthew, please do not combine code style changes with substantial changes. > That makes patches very hard to read. Separate them into 2 patches, in fact > this doesn't even belong in this bug. Ben, I appreciate your concern and generally it is of course better to separate this, but for calendar this is not a strict requirement. Lightning doesn't have too many contributors and I'd rather have a few extra lines in the patch than that extra patch never being created because its more and mostly annoying work. In the past I have asked people to do let/var changes within the same function of the lines they change, as long as its just a few. The other alternative is to munge history a bit by creating a great big patch that does let/var changes.

Places I found which rely on calISyncWriteCalendar to actually be synchronous: http://mxr.mozilla.org/comm-central/source/calendar/providers/gdata/components/calGoogleCalendar.js#1035 ( will be removed likely when v3 api is used, some time this month) http://mxr.mozilla.org/comm-central/source/calendar/providers/caldav/calDavCalendar.js#1114 http://mxr.mozilla.org/comm-central/source/calendar/providers/caldav/calDavRequestHandlers.js#124 There might be some other places still, and given the postPone stuff those providers might already be broken. We should eventually move to using promises and Task.jsm, to clean up the messy callback code. With the caldav provider we should also eventually move to rewriting it to use the b2g caldav library, but thats...another (set of) bugs ;-)

Comment on attachment 833396 [details] [diff] [review] Part 1: memory provider v2 The code itself looks fine, but I urge you to test those situations in the caldav provider before pushing.

Matt, how is this going? I think it would be nice to move this forward.

(In reply to Philipp Kewisch [:Fallen] from comment #10) > Places I found which rely on calISyncWriteCalendar to actually be > synchronous: > > http://mxr.mozilla.org/comm-central/source/calendar/providers/gdata/ > components/calGoogleCalendar.js#1035 > ( will be removed likely when v3 api is used, some time this month) > > > http://mxr.mozilla.org/comm-central/source/calendar/providers/caldav/ > calDavCalendar.js#1114 > http://mxr.mozilla.org/comm-central/source/calendar/providers/caldav/ > calDavRequestHandlers.js#124 > As far as I can see these places aren't affected by the changes, since they only call into the singular getItem function.

Pushed to comm-central: https://hg.mozilla.org/comm-central/rev/37b2288eb70b