CalTimezoneService accesses calendar.icaljs preference on a lot
Categories
(Calendar :: General, defect)
Tracking
(thunderbird_esr78+ fixed, thunderbird81 fixed)
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Keywords: perf)
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
wsmwk
:
approval-comm-beta+
wsmwk
:
approval-comm-esr78+
|
Details | Review |
CalTimezoneService accesses the calendar.icaljs
preference a lot I noticed this happening on a mochitest startup where the preference was accessed more than 40 times.
This can easily be replaced by a lazy preference getter.
Assignee | ||
Comment 1•4 years ago
|
||
Updated•4 years ago
|
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/comm-central/rev/b8678d458a9f
Cache calendar.icaljs value in CalTimezoneService as it is accessed a lot. r=pmorris
Assignee | ||
Updated•4 years ago
|
Comment 3•4 years ago
|
||
We should get this uplifted.
Assignee | ||
Comment 4•4 years ago
|
||
Comment on attachment 9171595 [details]
Bug 1660702 - Cache calendar.icaljs value in CalTimezoneService as it is accessed a lot. r?pmorris!
We should get this uplifted.
Not totally sure how much this will help, but I suspect it'll help a bit with large calendars.
[Approval Request Comment]
Regression caused by (bug #): Bug 869407 / Bug 871269
User impact if declined: Accessing preferences multiple times tends to be slow and unnecessary. Should help a little with performance, unsure how much.
Testing completed (on c-c, etc.): Landed on c-c.
Risk to taking this patch (and alternatives if risky): Low - uses well proven functions.
Comment 5•4 years ago
|
||
Comment on attachment 9171595 [details]
Bug 1660702 - Cache calendar.icaljs value in CalTimezoneService as it is accessed a lot. r?pmorris!
[Triage Comment]
Approved for beta
Comment 6•4 years ago
|
||
bugherder uplift |
Thunderbird 81.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/f52914ee2b56
Updated•4 years ago
|
Comment 7•4 years ago
|
||
Comment on attachment 9171595 [details]
Bug 1660702 - Cache calendar.icaljs value in CalTimezoneService as it is accessed a lot. r?pmorris!
[Triage Comment]
Approved for esr78.
Richard, want to try a candidate build?
Comment 8•4 years ago
|
||
bugherder uplift |
Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/68c84860844b
Comment 9•4 years ago
•
|
||
(In reply to Wayne Mery (:wsmwk) from comment #7)
Richard, want to try a candidate build?
Where can it be found?
Comment 10•4 years ago
•
|
||
On a separate note why not also use lazyloading for other modules lodead in the same file? Would probably need to be done in a sepatate follow-up bug... loading them via XPCOMUtils lazy functions may also have the advantage of them being cached if I understood correctly... so modules would effectively be loaded only once only if needed and cached for reuse... may be handy perhaps... especially for calendar feature...
Example available in 1658026 as spotted and worked on by Mark already...
Assignee | ||
Comment 12•4 years ago
|
||
Just to note I suspect this fix won't have a tremendous impact. It might save a little bit on a really long load, but we'll see.
(In reply to Richard Leger from comment #10)
On a separate note why not also use lazyloading for other modules lodead in the same file? Would probably need to be done in a sepatate follow-up bug... loading them via XPCOMUtils lazy functions may also have the advantage of them being cached if I understood correctly... so modules would effectively be loaded only once only if needed and cached for reuse... may be handy perhaps... especially for calendar feature...
Example available in 1658026 as spotted and worked on by Mark already...
All jsm modules are cached, even when not loaded lazily. The advantage of lazy loading is that they're not loaded until needed, which can help startup through not loading some things and/or loading things later when the disk is less busy. It can also help general use memory if features are loaded only when actually used.
Bug 1659558 isn't about lazy loading, that's about how some objects are created, which is currently using an expensive route and so should see a significant improvement (based on my testing). The rest of the calendar perf issues really seem to be engrained in the architecture so may be harder to solve.
Overall, I'd suspect lazy loading would have a smallish affect on startup, it should still be done (across all of TB, not just calendar), but where your profile from bug 1649103 was about 16s, I'd be surprised if it saved more than a second on that.
Comment 13•4 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #12)
Just to note I suspect this fix won't have a tremendous impact. It might save a little bit on a really long load, but we'll see.
The rest of the calendar perf issues really seem to be engrained in the architecture so may be harder to solve.
I don't believe it is necessarily the all architectute but Bug 1642292 remain the main culprit for calendar loading perf... until this is fixed I don't think anything else would make a noticable difference... though still valuable... any perf gain is worth it!
Thanks for helping TB run faster... any little gain does count... at least I am glad to see that perf issues are beeing tackled ;-)
Comment 14•4 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #12)
Bug 1659558 isn't about lazy loading,
What I meant is that you used "XPCOMUtils.defineLazyModuleGetters" as part of your fix there... to replace "ChromeUtils.import" calls which seems more expensive from perf point of view (if I understood correctly). But it is not the case here...
Problem with calendar is that in email view, the envents&tasks panel is often opened by default so triggerring calendar feature loading at startup while email view populates... causing slowdown...
Assignee | ||
Comment 16•4 years ago
|
||
(In reply to Richard Leger from comment #14)
(In reply to Mark Banner (:standard8) from comment #12)
Bug 1659558 isn't about lazy loading,
What I meant is that you used "XPCOMUtils.defineLazyModuleGetters" as part of your fix there... to replace "ChromeUtils.import" calls which seems more expensive from perf point of view (if I understood correctly). But it is not the case here...
It is all about delaying loading until needed. The XPCOMUtils import that I added here is not worth lazy loading because it is used straight away. Some of the other imports could potentially be lazy loaded, but really you're not going to see any gains until we work on segments of the whole code base, and the fact that Calendar loads all its data before UI startup, is likely to load all the modules anyway.
I do aim to file a bug at some stage for improving lazy loading across Thunderbird, I think it'd be better done as a group rather than individual files.
Comment 17•4 years ago
•
|
||
(In reply to Mark Banner (:standard8) from comment #16)
I do aim to file a bug at some stage for improving lazy loading across Thunderbird, I think it'd be better done as a group rather than individual files.
If you do, I would suggest to create a meta bug and then possibly separate bugs it depends on for group of files... so it can be achieved in a more atomic level over iterations... succession of small changes rather than a large one... this way it is also a small succession of small jobs... easier to find time for it than thinking all needs to be done at once ;-) It may also accelerate review and landing and easier to fix testing errors or bugs later on down the line... I suppose...
Just a suggestion... other may have different views...
Comment 18•4 years ago
|
||
Here is a perf profile from 78.2.2 (64-bit):
https://share.firefox.dev/300j1bD
TB starting with just one caldav network calendar setup (~4000+ items) but not loading properly for some reason... but that should at least give you info about what your looking for hopefully...
Description
•