1590665 - For recurring events, skip calculating occurrences if they're known to be outside the date range requested

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, enhancement)

Description

[Geoff Lankow (:darktrojan)]

When calling getItems on a calendar, occurrences are calculated for every recurring event, to see if there are any between aRangeStart and aRangeEnd. This is a waste of time in a lot of cases, because we can know that there are no occurrences. For example, past events that have stopped recurring.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1590665-recurrence-end-1.diff

Philipp what do you think of this? It's more a proof of concept than a finished patch, but it demonstrates what I have in mind.

The idea is that the last possible date is remembered (from the first call to getItems at this stage) and used to ignore the items in subsequent calls to getItems. This could be extended further: the current calculation is not as good as it could be, and I think we could go as far as storing the information in the database and not even retrieving irrelevant items from it.

In the limited testing I've done with this patch it is a good performance win.

Comment 2

[Geoff Lankow (:darktrojan)]

In the limited testing I've done with this patch it is a good performance win.

It really depends on what's in your calendars but I've seen this take a third off the time taken to go back or forward in the month view.

Comment 3

[Philipp Kewisch [:Fallen][☁️📆]]

I seem to recall the "previous" iterator wasn't very efficient, maybe double check the implementation? You should also check the edge cases of different rules, e.g. using COUNT or UNTIL.

The other thing we could do to speed this up, I have some code that is super fast and is capable of returning all occurrences within a specified interval, though no getting next/previous occurrences. It isn't my code but the author has indicated he'd be willing to re-license if necessary.

Comment 4

[Geoff Lankow (:darktrojan)]

I'm not really interested yet in how we calculate the last occurrence, as long as it's right.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1590665-recurrence-end-2.diff

Even with a poorly performing implementation, this provides such an improvement I think it's worth having. I've changed it so that only immutable objects are optimised (assuming bug 1601848 is fixed) so that we don't have to deal with the implications of a possibly-changing value. Mutable objects would see little benefit anyway, IMO.

Comment 6

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 9114472 [details] [diff] [review]
1590665-recurrence-end-2.diff

Review of attachment 9114472 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/src/calRecurrenceInfo.js
@@ +139,5 @@
> +    // If this object is mutable, skip this optimisation, so that we don't have to work out every
> +    // possible modification and invalidate the cached value. Immutable objects are unlikely to
> +    // exist for long enough to really benefit anyway.
> +    if (this.isMutable) {
> +      return 0x7ffffffffffffdff;

Shouldn't this be 0x7fffffffffffffff ? Maybe a constant would make sense.

::: calendar/test/unit/test_recur.js
@@ +22,5 @@
>    test_immutable();
>  }
>  
>  function test_rules() {
> +  function check_recur(event, expected, endDate, ignoreNextOccCheck) {

If you are using cal.createDateTime("...").nativeTime on every call, maybe it makes sense to just pass the string date and then turn it into a PRTime in this function?

Comment 7

[Geoff Lankow (:darktrojan)]

0x7fffffffffffffff is a negative number when converted to PRTime (a.k.a. 64-bit integer), because in javascript it's stored as a floating-point number, loses precision, and is greater than 2⁶³.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/83cf86aba5a2
For recurring events, skip calculating occurrences if they're known to be earlier than the date range requested; r=Fallen

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0c56d5bd0e5d
follow-up - Undo last-minute change which broke everything; rs=me

Comment 10

[Philipp Kewisch [:Fallen][☁️📆]]

Interesting, we use 0x7fffffffffffffff in a few other places. Maybe worth checking out :)