Closed Bug 1659558 Opened 3 months ago Closed 2 months ago

Calendar's use of createInstance to create instances of javascript objects is expensive

Categories

(Calendar :: General, defect)

defect

Tracking

(thunderbird_esr78 wontfix, thunderbird81 wontfix)

RESOLVED FIXED
82 Branch
Tracking Status
thunderbird_esr78 --- wontfix
thunderbird81 --- wontfix

People

(Reporter: standard8, Assigned: pmorris)

References

(Blocks 3 open bugs)

Details

(Keywords: perf)

Attachments

(9 files, 2 obsolete files)

9.51 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

Calendar is currently using some wrappers around createInstance in calUtils.jsm to create all of its calAlarm, calEvents, etc objects.

As seen in the marker chart of this performance profile, there's lots of calls to ChromeUtils.import for calendar objects.

If we do a simple rewrite in CalStorageCalendar.jsm to create new instances of the actual javascript objects, and initialise appropriately, then on my test profile I see approximately a drop in startup time from 13 seconds to 10 seconds (~23% improvement).

The profile afterwards can be seen here with a lot less calls to ChromeUtils.import https://share.firefox.dev/3256xPU

This is the patch I used to generate the improvement. It creates the classes direct, initialising them as appropriate.

I did look at replacing cal's _instance method with one that created the object, however I then ran into some weird issues that looked like cyclic import issues.

I consider this incomplete because:

  • CalRecurrenceRule isn't included. This is not currently in a jsm file, and so it doesn't seem to get exported. Though I'm not sure if it is feasible to change that at this stage.
  • It might be better to change the various Javascript objects (calAlarms etc) to pass the item for initialisation straight into the constructor. This would avoid the need to do a separate call and hence avoid the desire for wrapper functions.
  • It would probably be a good idea to replace all the cal.create* functions with direct initialisation of the Javascript objects. I'm not sure if this would want to be done here or elsewhere.
Attachment #9170493 - Attachment is patch: true

Philipp, Paul, generally I think this is mostly done, apart from the issues mentioned in the previous comment.

I'm probably not going to really have more time to spend on this for a bit now, though I'd really like to see this land given how much it helps. If there's just a couple of minor changes on the existing patch, I'd happily do them, but anything more I'd really like to hand-off to someone on the calendar team.

Flags: needinfo?(philipp)
Flags: needinfo?(paul)
See Also: → 1659582

Thanks for this! It's a relatively easy way to improve performance and we could use some of that. So I'm inclined to say we should go for it. I know Philipp has in mind a general de-xpcom effort for calendar, and this is basically a step in that direction. So we may want to think about how this fits into that plan, and whether we'd want to do anything a bit differently to align with that. Also, we have usually kept things under the cal namespace and this moves away from that. My main concern is to keep consistency in how these objects are created, (which is your third bullet in comment 1). Curious to hear Philipp's thoughts.

Personally I don't think the cal. home made namespacing makes any sense. It's non-ideomatic to js and I haven't seen anything like that used elsewhere around the codebase.

Assignee: nobody → paul
See Also: → 1642292

(In reply to Magnus Melin [:mkmelin] from comment #4)

Personally I don't think the cal. home made namespacing makes any sense. It's non-ideomatic to js and I haven't seen anything like that used elsewhere around the codebase.

Agreed, I don't know anywhere else that groups object creation like that. Services/MailServices are only gathered together as that was a performance optimisation as it avoided doing getService every time (which also is an expensive call).

If you did really want to keep it, then I think you'd need to stop using cal for some of the other things which cause a cycle.

Severity: -- → S3
Keywords: perf

Personally, I don't have strong opinions about dropping or keeping the cal namespacing, but I think Philipp has some thoughts on that.

Flags: needinfo?(paul)

The namespacing in cal is certainly unique, but I don't feel it is any more difficult to mentally parse than loading modules. It does have the advantage that calling functions in these modules is more explicit. If for example setDefaultStartEndHour shows up randomly in the code, it isn't clear if this is a local function and you have to double check at the top that it was imported from var { setDefaultStartEndHour } = ChromeUtils.import("..."); If you call it via cal.dtz.setDefaultStartEndHour() it is clearly from the dtz module. It also avoids name clashes for functions with more simple names.

On the other hand, there may be a slight overhead in accessing object members for cal and dtz, and I don't know if there is a negative impact to having everything contained somewhere in the cal object. Right now I feel this is more a matter of taste, but this is not a hill I'm going to die on.

The cyclic issues can usually be resolved by lazy-loading calUtils and checking that cal isn't accessed toplevel.

Flags: needinfo?(philipp)

In general, it's pretty ugly when modules export functions and not the "wrapper class" - that results in exactly the kind of confusion you mention. I'd like to see such usages converted, e.g. setDefaultStartEndHour which is currently in calDateTimeUtils.jsm which exports that single function.

Preferably that should be in an capitalized filename and exported object, so that it's obvious in the source what is variables (lower case) and what is the module (upper case). Then to use it one would import CalDateTimeUtils.jsm which would export a CalDateTimeUtils, so for usage CalDateTimeUtils.setDefaultStartEndHour would be called.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #7)

The namespacing in cal is certainly unique, but I don't feel it is any more difficult to mentally parse than loading modules. It does have the advantage that calling functions in these modules is more explicit. If for example setDefaultStartEndHour shows up randomly in the code, it isn't clear if this is a local function and you have to double check at the top that it was imported from var { setDefaultStartEndHour } = ChromeUtils.import("..."); If you call it via cal.dtz.setDefaultStartEndHour() it is clearly from the dtz module. It also avoids name clashes for functions with more simple names.

I don't think you necessarily need to remove all of the cal namespace for this bug. Only the various cal.create* functions. You can decide on the future of the namespace later. For me the important part here is speeding up startup when you have a calendar installed.

The objects being created are already named Cal* so they already have a good indication that they're part of calendar.

On the other hand, there may be a slight overhead in accessing object members for cal and dtz, and I don't know if there is a negative impact to having everything contained somewhere in the cal object. Right now I feel this is more a matter of taste, but this is not a hill I'm going to die on.

I know in the past I've seen comments about dot-accesses in objects being relatively expensive - but mix that with ChromeUtils.import trade-offs and it is probably swings and roundabouts as to what's better (maybe slightly lower perf overall vs slightly lower perf on startup).

The cyclic issues can usually be resolved by lazy-loading calUtils and checking that cal isn't accessed toplevel.

That doesn't work for some of the Cal* objects currently - they need cal at the top-level because of the QI and classInfo generation. Unfortunately those objects are probably the ones that really need fixing as they're the ones created most often.

After looking closer at this, and given the cyclic issues, I think it makes sense to drop the cal.create* approach and just do new Cal*. And also "change the various Javascript objects (calAlarms etc) to pass the item for initialisation straight into the constructor". A few of the constructors already take an argument, so will need to start taking two. This seems simpler to me, more idiomatic to JS, and will help with startup performance. No longer being under the cal namespace seems fine given that the objects/classes/constructors in question are all prefixed with Cal* and are typically the only thing exported from their modules.

I'm working on some patches that will carry out this conversion everywhere in calendar.

This fixes the test_unifinder_utils.js xpcshell test.

Depends on D89268

Depends on D89271

The failing tests: test_alarm.js, test_alarmutils.js, test_items.js

Depends on D89273

(In reply to Paul Morris [:pmorris] from comment #18)

Created attachment 9173919 [details]
Bug 1659558 - Part 8: Fix a few CalAlarm related xpcshell tests. r=darktrojan

If you do many changesets, please remember that they should each really be atomic. I.e. this is one part that should be folded into whichever parent is causing the problem to arise. Without atomicity there's little point doing separate commits.

(In reply to Magnus Melin [:mkmelin] from comment #21)

If you do many changesets, please remember that they should each really be atomic. I.e. this is one part that should be folded into whichever parent is causing the problem to arise. Without atomicity there's little point doing separate commits.

Yeah, good point. My motivation was to make them a little easier to review by separating out the various test fixes, but having atomic commits is better for all other purposes. I can fold them into the parent commit before they land.

Attachment #9173914 - Attachment description: Bug 1659558 - Part 3: Handle defaults in makeMemberAttr function. r=darktrojan → Bug 1659558 - Part 1: Handle defaults in makeMemberAttr function. r=darktrojan
Attachment #9173912 - Attachment description: Bug 1659558 - Part 1: Convert `cal.createEvent()` to `new CalEvent()`. r=darktrojan → Bug 1659558 - Part 2: Convert `cal.createEvent()` to `new CalEvent()`. r=darktrojan
Attachment #9173913 - Attachment description: Bug 1659558 - Part 2: Convert `cal.createTodo()` to `new CalTodo()`. r=darktrojan → Bug 1659558 - Part 3: Convert `cal.createTodo()` to `new CalTodo()`. r=darktrojan
Attachment #9173918 - Attachment description: Bug 1659558 - Part 7: Convert `cal.createAlarm()` to `new CalAlarm()`. r=darktrojan → Bug 1659558 - Part 6: Convert `cal.createAlarm()` to `new CalAlarm()`. r=darktrojan
Attachment #9173920 - Attachment description: Bug 1659558 - Part 9: Convert `cal.createRelation()` to `new CalRelation()`. r=darktrojan → Bug 1659558 - Part 7: Convert `cal.createRelation()` to `new CalRelation()`. r=darktrojan
Attachment #9173921 - Attachment description: Bug 1659558 - Part 10: Convert `cal.createRecurrenceInfo()` to `new CalRecurrenceInfo()`. r=darktrojan → Bug 1659558 - Part 8: Convert `cal.createRecurrenceInfo()` to `new CalRecurrenceInfo()`. r=darktrojan
Attachment #9173917 - Attachment is obsolete: true
Attachment #9173919 - Attachment is obsolete: true

I've folded the test fix patches into the other patches and addressed the review comments. Re-requesting review on the set of changes that weren't yet approved. Try run just started: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=aec703358b3bf2119892c88b00f98446f0bb386a

Blocks: 1642292
Blocks: 1649103

I see the try run has some more failing tests to fix.

Status: NEW → ASSIGNED
Attachment #9173914 - Attachment description: Bug 1659558 - Part 1: Handle defaults in makeMemberAttr function. r=darktrojan → Bug 1659558 - Part 1: Refactor makeMemberAttr function. r=darktrojan

Some mochitests were failing because usually if you don't modify a new event or task and close it, there is no prompt to save it. But this prompt was appearing, causing the tests to break. The code that detects whether any changes have been made, and how it uses the code for cloning a calendar item were not working as expected. (see isItemChanged in lightning-item-iframe.js)

Starting to support setting up property defaults in the makeMemberAttr function was causing the problem. The default for CLASS/privacy ("PUBLIC") that we started setting up in makeMemberAttr isn't added to mProperties and the cloning code just iterates through mProperties making copies of them. So that default wasn't being cloned.

But in the event dialog, the privacy property was accessed directly as event.privacy when loading the event, returning the default value. So the cloned event didn't have a privacy value, but the event in the dialog now did. That meant the check for whether anything had changed always said that something had changed, prompting the user about saving changes and breaking tests.

I've fixed this by not allowing setting default values in the makeMemberAttr function when using getProperty/setProperty (to avoid surprises when cloning objects). Those default values can just be assigned during construction of the objects, when needed. Since setting up defaults in makeMemberAttr wasn't even implemented before, it seems the existing code doesn't require these defaults to be set up there.

I found that a default percentComplete on tasks was needed for the xpcshell tests. Passing in an icalString to the constructor that didn't have a 'PERCENT-COMPLETE' would result in percentComplete being null instead of 0.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c1bc5607d2b6a0e250a67b77c6fddebc6c7990c5
Requesting review of the revised 'part 1' patch.

Pushed by paul@paulwmorris.com:
https://hg.mozilla.org/comm-central/rev/54552b835a0a
Part 1: Refactor makeMemberAttr function. r=darktrojan
https://hg.mozilla.org/comm-central/rev/6b980081e627
Part 2: Convert cal.createEvent() to new CalEvent(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/13cd385e74bb
Part 3: Convert cal.createTodo() to new CalTodo(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/157163215e76
Part 4: Convert cal.createAttendee() to new CalAttendee(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/3f0a857f8760
Part 5: Convert cal.createAttachment() to new CalAttachment(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/8f288f9c7ef3
Part 6: Convert cal.createAlarm() to new CalAlarm(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/7219f503aded
Part 7: Convert cal.createRelation() to new CalRelation(). r=darktrojan
https://hg.mozilla.org/comm-central/rev/70602de6d034
Part 8: Convert cal.createRecurrenceInfo() to new CalRecurrenceInfo(). r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
You need to log in before you can comment on or make changes to this bug.