315959 - profile ICS calendar reading

Status: RESOLVED FIXED

(Calendar :: Provider: ICS/WebDAV, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

And pick off any low-hanging fruit before shipping 0.1, since performance is currently almost unusably slow.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Using quick-and-dirty profiling, the only bug consumer i found was clone() on the item (in calMemoryProvider.js, addItem()). The rest of the time was spend on all kind of places.
I could make it so that mutable items are not cloned, but i'm not sure if that is the right thing to do. It would mean that the item you give will be modified.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Do you mean that every mutable item handed in would be modified?  Or just ones where someone later called modifyItem?  If the former, in what way would they be modified?

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Every mutable item that gets added, will be modified. At least .parent will get changed, and .generation might change.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

In the general case of adding events, I think cloning is the right thing to do.  However, in the specific case of populating a memory calendar from some provider datasource, it seems fairly clearly unnecessary.  Perhaps we need some API bit that allows us to indicate that cloning isn't necessary and that we're transferring event ownership to the memory calendar.  I'm not sure exactly where the best place for such an API frob would be.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

I'm thinking of a adoptItem in calICalendar, next to addItem. For most calendars, addItem will clone the item, and then call adoptItem.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: add adoptItem

patch adds adoptItem. If we go with this, it should be implemented for the other calendars, but i left it out for now, to keep the patch a bit smaller. Once this patch has review+, i will create one with the function implemented for other calendars, and ask for a quick review on that.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 208405 [details] [diff] [review]
add adoptItem

Looks good; r=dmose.  It might be worth adding a comment about this generally being intended for performance critical stuff.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

I switched from my debug-build-with-gcc setup to using the branch nightlies with Thunderbird 1.5 on my slowest machine (which is a 1.5 Ghz Win XP box), and things aren't exactly snappy, but they're well within limits for a 0.1 release.  Following Simon's advice and removing this from the Lightning 0.1 blocker list.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: full patch (checked in)

This is the full patch, as promised.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 208767 [details] [diff] [review]
full patch (checked in)

>+   * adoptItem adds the given calIItemBase to the calendar, but doesn't
>+   * clone it. It adopts the item as-is. This is to be used in performance-
>+   * critical situations.
>+   *

How about: "This is generally for use in performance-critical situations where there is no danger of the caller using the item after making the call." ?

Nice work!

r=dmose

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 208767 [details] [diff] [review]
full patch (checked in)

Patch checked in, leaving bug open for more profiling work.

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to Michiel van Leeuwen (email: mvl+moz@) from comment #11)
> Comment on attachment 208767 [details] [diff] [review]
> full patch (checked in)
> 
> Patch checked in, leaving bug open for more profiling work.

on IRC with fallen, we can "open new ones if we find new ways to profile"

so resolving this as FIXED by comment 11. If someone wants to set TM please do so