408264 - Use cached DTSTART

Status: RESOLVED DUPLICATE of bug 685884

(Calendar :: General, defect)

Description

[Sebastian Schwieger]

Attachment: cache startDate and entryDate

Caching event.startDate and todo.entryDate improved a profiling run using Simons steps he reported on the Newsgroup [1] from 3169 ms to 265 ms (time for get startDate()). This is by far not not the main issue of the reported times but it improves things a little bit.

[1] http://groups.google.de/group/mozilla.dev.apps.calendar/browse_thread/thread/0f5fd487ac38ced8#20c4823deb717199

Comment 1

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

Let's just fix this is general: bug 382121.

Comment 2

[Daniel Boelzle [:dbo]]

Comment on attachment 293008 [details] [diff] [review]
cache startDate and entryDate

I am sure property bag improvements are needed, but even then if the startDate attribute seems to be a hot spot, I think it makes sense to improve it as much as possible.

However, I have some nits

>+    mStartDate: null,
...
>     get startDate() {
>-        return this.getProperty("DTSTART");
>+        if (!this.mStartDate) {
>+            this.mStartDate = this.getProperty("DTSTART");
>+        }
>+        return this.mStartDate;

This will init mStartDate over and over again if that property is not existing, e.g. consider tasks without DTSTART. I'd prefer to use an initial "undefined" and strongly check against that:

if (this.mStartDate === undefined) { ...


Sebo, please check the code base for any setProperty("DTSTART"), because that won't update the cached values.

r- for now, because of the init issue

Comment 3

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

The patch in bug 382121 does almost the same as this one here, except it's general. It would even hurt to take the patch here, if the property bag patch is taken. Then you would have a cache that's based on the same code as the real storage. That won't speed things up, but would just eat memory.
Why not take the general route? It will fix the same hotspot, and even some more. Special-casing like this is just not needed.

Comment 4

[Daniel Boelzle [:dbo]]

mvl, you are right. IMO we should first fix bug 382121 (I set that to wanted0.8) and see whether there is still a hot spot with startDate/entryDate. If it turns out this this problem persists, we need to optimize further, e.g. in calEvent/calTodo.

Comment 5

[Daniel Boelzle [:dbo]]

Sebo, could you run your profiler again whether this hotspot still persists, please?

Comment 6

[Sebastian Späth]

Can we get new profiler run or close this bug now as fixed, please? Underlying issue seems to have been fixed and this bug lingering since 2 years

Comment 7

[Michael Duelli]

I would offer to reproduce the steps done in http://groups.google.de/group/mozilla.dev.apps.calendar/browse_thread/thread/0f5fd487ac38ced8#20c4823deb717199
from comment #1 with current lightning nightly.

However, the link for the reference .ics file given in there is dead. Any other sources?

Comment 8

[Bas van den Bosch]

Michael, It should be reproducable with any ics...

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Michael, can you take another look? Bas is right, any ics calendar that contains a fair amount of events should do.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think we can dupe this one against bug 685884. The intent there is to do quite the opposite, use the components directly instead of initializing yet another layer of caching. While back when this bug was filed the problem was crossing the xpcom border, with ical.js this will be different.