Last Comment Bug 315959 - profile ICS calendar reading
: profile ICS calendar reading
Status: RESOLVED FIXED
[not needed beta][no l10n impact]
: perf
Product: Calendar
Classification: Client Software
Component: Provider: ICS/WebDAV (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Michiel van Leeuwen (email: mvl+moz@)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-10 16:18 PST by Dan Mosedale (:dmose)
Modified: 2012-11-15 05:47 PST (History)
5 users (show)
philipp: blocking‑calendar1.0-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add adoptItem (6.61 KB, patch)
2006-01-13 12:32 PST, Michiel van Leeuwen (email: mvl+moz@)
dmose: first‑review+
Details | Diff | Splinter Review
full patch (checked in) (12.45 KB, patch)
2006-01-17 12:22 PST, Michiel van Leeuwen (email: mvl+moz@)
dmose: first‑review+
Details | Diff | Splinter Review

Description Dan Mosedale (:dmose) 2005-11-10 16:18:04 PST
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@) 2006-01-08 04:38:02 PST
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 (:dmose) 2006-01-10 11:23:17 PST
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@) 2006-01-10 13:06:15 PST
Every mutable item that gets added, will be modified. At least .parent will get changed, and .generation might change.
Comment 4 Dan Mosedale (:dmose) 2006-01-11 15:03:13 PST
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@) 2006-01-13 11:33:59 PST
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@) 2006-01-13 12:32:37 PST
Created attachment 208405 [details] [diff] [review]
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 (:dmose) 2006-01-17 11:18:29 PST
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 (:dmose) 2006-01-17 12:12:29 PST
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@) 2006-01-17 12:22:44 PST
Created attachment 208767 [details] [diff] [review]
full patch (checked in)

This is the full patch, as promised.
Comment 10 Dan Mosedale (:dmose) 2006-01-17 13:02:16 PST
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@) 2006-01-19 13:56:57 PST
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, NI for questions) 2012-11-15 05:47:51 PST
(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

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