The default bug view has changed. See this FAQ.

profile ICS calendar reading

RESOLVED FIXED

Status

Calendar
Provider: ICS/WebDAV
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: dmose, Assigned: Michiel van Leeuwen (email: mvl+moz@))

Tracking

({perf})

Bug Flags:
blocking-calendar1.0 -

Details

(Whiteboard: [not needed beta][no l10n impact])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
And pick off any low-hanging fruit before shipping 0.1, since performance is currently almost unusably slow.
(Reporter)

Updated

12 years ago
Summary: need to profile non-trivially sized ICS calendar reading performance → profile ICS calendar reading
(Assignee)

Comment 1

11 years ago
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.
(Reporter)

Comment 2

11 years ago
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?
(Assignee)

Comment 3

11 years ago
Every mutable item that gets added, will be modified. At least .parent will get changed, and .generation might change.
(Reporter)

Comment 4

11 years ago
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.
(Assignee)

Comment 5

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

Comment 6

11 years ago
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.
Attachment #208405 - Flags: first-review?(dmose)
(Reporter)

Comment 7

11 years ago
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.
Attachment #208405 - Flags: first-review?(dmose) → first-review+
(Reporter)

Comment 8

11 years ago
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.
No longer blocks: 298366
(Assignee)

Comment 9

11 years ago
Created attachment 208767 [details] [diff] [review]
full patch (checked in)

This is the full patch, as promised.
Attachment #208405 - Attachment is obsolete: true
Attachment #208767 - Flags: first-review?(dmose)
(Reporter)

Comment 10

11 years ago
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
Attachment #208767 - Flags: first-review?(dmose) → first-review+
(Assignee)

Comment 11

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

Patch checked in, leaving bug open for more profiling work.
Attachment #208767 - Attachment description: full patch → full patch (checked in)

Updated

11 years ago
Component: Lightning Only → Provider: ICS/Webdav
QA Contact: lightning → ics-provider

Updated

8 years ago
Keywords: helpwanted
Flags: blocking-calendar1.0+
Whiteboard: [not needed beta][no l10n impact]
Flags: blocking-calendar1.0+ → blocking-calendar1.0-
(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
Assignee: nobody → mvl
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Keywords: helpwanted
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.