270501 - the list of events for the listbox (unifinder) is sorted three times

Status: RESOLVED WORKSFORME

(Calendar :: General, defect)

Description

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

The list of events for displaying in the listbox is sorted three times before
the user sees it.
1. inside libxpical, by date
2. in function calEvent_getAllEvents (or one of it's friends), by date
3. in the actual display, by whatever the user selected.

The first two seem redundant, at least in this case. Are there cases where they
are needed? If so, can we pass a parameter or something, to skip sorting if not
needed?

Comment 1

[Calum Mackay]

with lots of entries, of course, (I have 1,200) this could explain some of the performance problems reported elsewhere.

Comment 2

[Joey Minta]

(In reply to comment #1)
> with lots of entries, of course, (I have 1,200) this could explain some of the
> performance problems reported elsewhere.
> 

This bug is pretty old, and our method of getting the events from a calendar has changed since it was filed.  In particular, (1) is gone and (2) is obsolete.  Events are now returned based on their next occurrence, which works, in a vague way, if the unifinder sort is based on start/end columns.  (I would need to look into the details of which column might use this.)  (3) still exists, and is necessary in all cases other than where we can re-use the sort.  Still, Array.protoype.sort(), which is what (3) uses, is reasonably fast.  There are other areas in the unifinder that have much more severe consequences on our performance.  See also wiki.mozilla.org/index.php?title=User:Jminta:Sunbird_Speed#Trees

mvl, any objection to closing this bug?

Comment 3

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

1 still exists, but is now insed the provider. Don't know about 2 (maybe the composite does some sorting). 3 also still exists.
Let's keep it open until we are sure.

Comment 4

[Matthew (lilmatt) Willis]

Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove

Comment 5

[Martin Schröder [:mschroeder]]

Unifinder is also available in Lightning now.

Comment 6

[Christophe Humbert]

Hello,

Is this bug still valid. if that is the case I am ready to work on it with some guidance.

Kind Regards

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Christophe, I'm glad you'd like to step up. I'm not sure how valid this still is, it requires some investigation. What you can do:

* Read some of the provider code (i.e storage/ics) to find out if there is some sorting being done.
* If applicable, read relevant libical code that retrieves items from an ics stream to see if it is sorted.

The mentioned point (3) is definitely needed, you'd need to find and comment out all sorting code in points (2) and (1). When you have this, check all places that call getItems() and see if they still do what they should.

Note that we don't really want to do code changes in libical itself. If its not possible to work around, we should file an upstream bug.

Comment 8

[Markus Adrario [:Taraman]]

I had a look at this and believe, it is not valid anymore.

for 1.:
is now libical. (comm-central/calendar/libical)
The array sorting is done in [1], and does not seem to be called automatically inside libical [2].

for 2.:
should be the getItems(...) functions of the providers now.
After having looked through [3], I can not see any sorting code there

for 3.:
This is, where sorting should happen. And it does, when needed for added items [4]

So I believe, this bug can be closed.

[1]: http://mxr.mozilla.org/comm-central/source/calendar/libical/src/libical/icalarray.c#127
[2]: http://mxr.mozilla.org/comm-central/ident?i=icalarray_sort
[3]: http://mxr.mozilla.org/comm-central/search?find=%2Fcalendar%2F&string=getItems
[4]: http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-unifinder.js#498

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Thanks for the investigation! Closing this bug as suggested.