841995 - getItems calls can block UI with large calendars

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Matthew Mecca [:mmecca]]

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.

Comment 1

[Matthew Mecca [:mmecca]]

Attachment: Part 1: memory provider

Comment 2

[Ben Bucksch (:BenB)]

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 3

[Philipp Kewisch [:Fallen][📱📆]]

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?

Comment 4

[Ben Bucksch (:BenB)]

> Did you check the actual performance win on this

Note that this is not about being faster, but about blocking, see comment 2.

Comment 5

[Matthew Mecca [:mmecca]]

(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 6

[Matthew Mecca [:mmecca]]

Attachment: Part 1: memory provider v2

Comment 7

[Ben Bucksch (:BenB)]

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 8

[Ben Bucksch (:BenB)]

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

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

(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.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

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 11

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

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

Comment 13

[Matthew Mecca [:mmecca]]

(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.

Comment 14

[Matthew Mecca [:mmecca]]

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