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

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Mark Banner (:standard8)]

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

Comment 1

[Mark Banner (:standard8)]

Attachment: Change CalStorageCalendar.jsm to create javascript objects directly (incomplete)

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.

Comment 2

[Mark Banner (:standard8)]

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.

Comment 3

[Paul Morris [:pmorris]]

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.

Comment 4

[Magnus Melin [:mkmelin]]

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.

Comment 5

[Mark Banner (:standard8)]

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

Comment 6

[Paul Morris [:pmorris]]

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

Comment 7

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

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.

Comment 8

[Magnus Melin [:mkmelin]]

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.

Comment 9

[Mark Banner (:standard8)]

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

Comment 10

[Paul Morris [:pmorris]]

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.

Comment 11

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 2: Convert `cal.createEvent()` to `new CalEvent()`. r=darktrojan

Comment 12

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 3: Convert `cal.createTodo()` to `new CalTodo()`. r=darktrojan

Depends on D89267

Comment 13

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 1: Refactor makeMemberAttr function. r=darktrojan

This fixes the test_unifinder_utils.js xpcshell test.

Depends on D89268

Comment 14

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 4: Convert `cal.createAttendee()` to `new CalAttendee()`. r=darktrojan

Depends on D89269

Comment 15

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 5: Convert `cal.createAttachment()` to `new CalAttachment()`. r=darktrojan

Depends on D89270

Comment 16

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 6: Fix test_alarm.js xpcshell test. r=darktrojan

Depends on D89271

Comment 17

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 6: Convert `cal.createAlarm()` to `new CalAlarm()`. r=darktrojan

Depends on D89272

Comment 18

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 8: Fix a few CalAlarm related xpcshell tests. r=darktrojan

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

Depends on D89273

Comment 19

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 7: Convert `cal.createRelation()` to `new CalRelation()`. r=darktrojan

Depends on D89274

Comment 20

[Paul Morris [:pmorris]]

Attachment: Bug 1659558 - Part 8: Convert `cal.createRecurrenceInfo()` to `new CalRecurrenceInfo()`. r=darktrojan

Depends on D89275

Comment 21

[Magnus Melin [:mkmelin]]

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

Comment 22

[Paul Morris [:pmorris]]

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

Comment 23

[Paul Morris [:pmorris]]

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

Comment 24

[Paul Morris [:pmorris]]

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

Comment 25

[Paul Morris [:pmorris]]

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.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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