Internal Components
14 years ago
12 years ago


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





(1 attachment)

I was trying to make importExport.js be a bit smarter, and create a oeIICal when
importing a .ics file, instead of parsing itself. I noticed it was slow. In
fact, it was 10 times as slow.
After some searching, i found that the problem is in
   m_eventlist.Add( icalevent );

If i remove that, it is suddenly 5 times as fast as the JS version.
Ofcourse, this is a real problem: once you are subscribed to a file, it is
parsed using libxpical, not using JS.

Comment 1

14 years ago
turns out i was wrong. GetAllEvents is what is slow. And without adding anything
to the array, it is ofcourse very fast...

Still, i now converted EventList m_eventlist into a nsVoidArray, which should
reduce codesize somewhat. Do we want that change?

Comment 2

14 years ago
The slugishness has been reported before, as bugs 187177, 255561 and 262209. I'm
not sure what's causing that but there certainly is some code that is not
efficient at all. An example ( and a suspect) is SetupAlarmManager() which
recalculates all alarms whenever a new event is added instead of keeping track
of the existing timers and adjusting them only if the new event has an alarm
that goes off sooner.
As for using a nsVoidArray in place of the Eventlist class, it's a good
improvement, please submit the patch. Can the GetEventById() and GetTodoById()
functions be somehow implemented in the same interface or would they need
out-of-class functions?

Comment 3

14 years ago
I reported this bug as a specifix thing that causes the slowness, not to cover
And the problem i want to look at here isn't the alarm manager. My testcase
pointed at GetAllEvents, but we can morph this bug into changing the linked list
into an array. I will attach a patch, and maybe file a new bug for GetAllEvents.

Comment 4

14 years ago
I added comments to the GetAllEvents() function hoping it will make the code
more readable and while at it I eliminated two redundant loops:

GetAllEvents() originally didn't have the sorting feature and just returned the
event list. The sortability was added hoping that it would eliminate the need
for sorting in the frontend (js) code and speed up the process. This however did
not happen for two reasons:
1) GetAllEvents() in oeICalContainer has to resort the list it gets from a call
to GetAllEvents() in individual calendars, and this is not yet implemented
2) The way JS does sorting using the orderRawEventsByDate function is neat and tidy

so now the sorting happens two times.
Maybe we should add a "sort" argument to GetAllEvents() to enable/disable
sorting and have it set to false for now just in case we want to move towards
this direction again in the future.

While targetting optimizations, I was thinking of adding a cached calculation
feature to events so that the result of calculations such as recurrence
calculations which are usually heavy can be stored in a variable and reused as
long as the event has not changed.
Summary: EventList.Add() is slow → GetAllEvents() is slow


14 years ago
Keywords: perf

Comment 5

14 years ago
Adding a parameter to disable sorting makes sense. I wanted to use oeIICal for
importing, and there i don't want the event to be sorted. No need to spend time
on it.

Comment 6

14 years ago
Created attachment 161704 [details] [diff] [review]
use nsVoidArray and quicksort

This patch turns m_eventlist into a nsVoidArray. This doesn't make it much
faster, it just removes codesize a little bit.
The good thing is that nsVoidArray provides a Sort(), that is quite fast. By
using that in GetAllEvents, it suddenly is 5-10x as fast.

(patch is -w for readability)

Comment 7

14 years ago
Comment on attachment 161704 [details] [diff] [review]
use nsVoidArray and quicksort

when this patch is reviewed etc, i will port it to m_todolist. Didn't do that
Attachment #161704 - Flags: first-review?(mostafah)

Comment 8

14 years ago
Comment on attachment 161704 [details] [diff] [review]
use nsVoidArray and quicksort

FetchEvent misses a pair of { } in the if(). imagine they are there.

Comment 9

13 years ago
Comment on attachment 161704 [details] [diff] [review]
use nsVoidArray and quicksort

Patch checked in.Thanks
Fixed indenting.
Added missing {}.
Attachment #161704 - Flags: first-review?(mostafah) → first-review+

Comment 10

13 years ago
Fixed in CVS.
Last Resolved: 13 years ago
Resolution: --- → FIXED
Mass move of libxpical bugs to the Internal Components, per ctalbert.
Component: libxpical → Internal Components
The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!
QA Contact: gurganbl → base
You need to log in before you can comment on or make changes to this bug.