Closed Bug 1590665 Opened 9 months ago Closed 7 months ago

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

Categories

(Calendar :: Provider: Local Storage, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 73.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

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.

Attached patch 1590665-recurrence-end-1.diff (obsolete) — Splinter Review

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.

Attachment #9103465 - Flags: feedback?(philipp)

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.

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.

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

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.

Attachment #9103465 - Attachment is obsolete: true
Attachment #9103465 - Flags: feedback?(philipp)
Attachment #9114472 - Flags: review?(philipp)
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?
Attachment #9114472 - Flags: review?(philipp) → review+

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⁶³.

Pushed by geoff@darktrojan.net:
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

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → 73
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/0c56d5bd0e5d
follow-up - Undo last-minute change which broke everything; rs=me

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

Regressions: 1633772
You need to log in before you can comment on or make changes to this bug.