Closed Bug 1086529 Opened 5 years ago Closed 5 years ago
[Template] Consider lazy initializing the Template library
I noticed we preallocate a lot of Template objects in the SMS app's startup code. These objects are currently initialized in the constructor: it means we extract data from the DOM and precalculate it. I thought it can be a good idea to lazy initialize these objects instead. I found a noticeable improvement in the SMS startup time after this change.
Hey David, how do you feel about this change? I didn't update any test because they already cover the API. Do you want a test for the "prepare" function?
Attachment #8508672 - Flags: review?(dflanagan)
I've uploaded a video showing the difference at https://www.youtube.com/watch?v=EQhLqr5gT_0 Left: without patch Right: with the patch We can see that with the patch it's either faster or as fast (sometimes it's slightly slower but I'm putting this on the statistic error margin).
Comment on attachment 8508672 [details] github PR I thought I had already reviewed and responded to this. I guess I had only reviewed it in my head. Sorry! I don't feel strongly about this either way. If it improves startup time, it is probably worth doing. I've noted one minor optimization opportunity on github. My main concern is that if there are any apps out there using this library that create their Template object and then remove the template comment they will break. So if you're going to land it like this, be sure to test carefully. Or consider making this an opt-in optimization where you have to pass true as the second argument to the Template() constructor to get the new behavior.
Attachment #8508672 - Flags: review?(dflanagan) → review+
(In reply to David Flanagan [:djf] from comment #3) > My main concern is that if there are any apps out there using this library > that create their Template object and then remove the template comment they > will break. Right, I'll carefully review all uses. Thanks!
I tested all cases where it's used, and it looks to still work fine :) Failures are unrelated, so squashing and adding checkin-needed to land when the tree reopens.
Adding a link with the right URL so that we see it's a github PR. Carrying over r=djf
Comment on attachment 8520005 [details] [review] github PR [Approval Request Comment] [Bug caused by] (feature/regressing bug #): - [User impact] if declined: worse performance at launch time for some applications [Testing completed]: yes, both automatic and manually [Risk to taking this patch] (and alternatives if risky): low by the size of the patch, medium because it can potentially impact several apps. I manually tested all cases where it's used to ensure it's not regressing. [String changes made]: -
Attachment #8520005 - Flags: approval-gaia-v2.1?(bbajaj)
How much do we gain with this patch?
Not a lot, but still enough to be noticeable with a side by side comparison (see video in comment 2). I'd say ~10ms.
(In reply to Julien Wajsberg [:julienw] from comment #10) > Not a lot, but still enough to be noticeable with a side by side comparison > (see video in comment 2). I'd say ~10ms. Julien given we've met the perf goals for 2.1 at this time, I think we can have all these nice improvements for 2.2 :) without risking any uplifts to 2.1 at this point in the release.
Attachment #8520005 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1-
You need to log in before you can comment on or make changes to this bug.