262620 - GetAllEvents() is slow

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

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

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

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

[Mostafa Hosseini]

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

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

I reported this bug as a specifix thing that causes the slowness, not to cover
everything.
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

[Mostafa Hosseini]

I added comments to the GetAllEvents() function hoping it will make the code
more readable and while at it I eliminated two redundant loops:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fcalendar%2Flibxpical&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-10-06+08%3A20&maxdate=2004-10-06+08%3A30&cvsroot=%2Fcvsroot

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.

Comment 5

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

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

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

Attachment: 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

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

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

Comment 8

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

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

[Mostafa Hosseini]

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

Patch checked in.Thanks
Fixed indenting.
Added missing {}.

Comment 10

[Mostafa Hosseini]

Fixed in CVS.

Comment 11

[Marcia Knous [:marcia]]

Mass move of libxpical bugs to the Internal Components, per ctalbert.

Comment 12

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!