1086529 - [Template] Consider lazy initializing the Template library

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Julien Wajsberg [:julienw]]

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.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: github PR

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?

Comment 2

[Julien Wajsberg [:julienw]]

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 3

[David Flanagan [:djf]]

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.

Comment 4

[Julien Wajsberg [:julienw]]

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

Comment 5

[Julien Wajsberg [:julienw]]

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.

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: github PR

Adding a link with the right URL so that we see it's a github PR.

Carrying over r=djf

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/c77f139ec97265a8b850dac2b6904f6bfc1d8902

Comment 8

[Julien Wajsberg [:julienw]]

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]: -

Comment 9

[[:fabrice] Fabrice Desré]

How much do we gain with this patch?

Comment 10

[Julien Wajsberg [:julienw]]

Not a lot, but still enough to be noticeable with a side by side comparison (see video in comment 2). I'd say ~10ms.

Comment 11

[bhavana bajaj [:bajaj]]

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