Closed Bug 1660702 Opened 4 years ago Closed 4 years ago

CalTimezoneService accesses calendar.icaljs preference on a lot

Categories

(Calendar :: General, defect)

defect

Tracking

(thunderbird_esr78+ fixed, thunderbird81 fixed)

RESOLVED FIXED
82 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird81 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: perf)

Attachments

(1 file)

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.

Status: NEW → ASSIGNED

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

We should get this uplifted.

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.

Attachment #9171595 - Flags: approval-comm-esr78?
Attachment #9171595 - Flags: approval-comm-beta?

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

Attachment #9171595 - Flags: approval-comm-beta? → approval-comm-beta+

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?

Flags: needinfo?(richard.leger)
Attachment #9171595 - Flags: approval-comm-esr78? → approval-comm-esr78+

(In reply to Wayne Mery (:wsmwk) from comment #7)

Richard, want to try a candidate build?

Where can it be found?

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

Flags: needinfo?(richard.leger)

Re-flaging for myself...

Flags: needinfo?(richard.leger)

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.

(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 ;-)

Flags: needinfo?(richard.leger)

(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...

Re-flaging for myself... (sorry)

Flags: needinfo?(richard.leger)

(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.

(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...

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

Flags: needinfo?(richard.leger)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: