Closed Bug 1118215 Opened 7 years ago Closed 7 years ago

[Messages] ThreadUI.init takes too much time


(Firefox OS Graveyard :: Gaia::SMS, defect)

Not set


(feature-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

2.2 S4 (23jan)
feature-b2g 2.2+
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed


(Reporter: julienw, Assigned: steveck)



(Whiteboard: [p=1])


(1 file)

46 bytes, text/x-github-pull-request
: review+
: feedback+
Details | Review
ThreadUI.init takes ~80ms to execute on a Buri. We need to find what is the culprit (one guess is Compose.init, but we need to check) and either make it less costly or move to beforeEnter.
Target Milestone: --- → 2.2 S4 (23jan)
NI Wesley to put the feature-b2g flag
Flags: needinfo?(whuang)
This is for app launch time improvement, so let's put feature-b2g:2.2+.
feature-b2g: --- → 2.2+
Flags: needinfo?(whuang)
Assignee: nobody → schung
After some investigation, getComputedStyle and focus in recipients should be the root cause here.
Attached file Link to github
Here is  some profiling result for ThreadUI init: The overall ThreadUI init takes ~30ms, and initRecipient takes half of time(~15ms). More precisely, most of time were spent on getPropertyValue(7ms or more) and node focus.

Hi Oleg, I simply remove the initRecipients from init in this patch because beforeEnterComposer will call it again. The main difference is ThreadUI.recipients will not exist if user don't enter compose view(and some necessary checking to prevent exception). Any feedback about this change(or further getPropertyValue revomal, because I can't find a better way to replace it) is welcome!
Attachment #8548679 - Flags: feedback?(azasypkin)
Comment on attachment 8548679 [details] [review]
Link to github

Looks good, 10-15ms is a nice win for a very small change (and architecturally correct in my opinion), thanks!

Agree that the the next step (not in this bug for sure) would be to try to deal with "getComputedStyle".
Attachment #8548679 - Flags: feedback?(azasypkin) → feedback+
Comment on attachment 8548679 [details] [review]
Link to github

Patch with unit tests, In ThreadUI tests I didn't initRecipient at first because not every case will need initialize recipients first. Tests should init by themselves if they really need test for recipients related scenario.
Attachment #8548679 - Flags: review?(azasypkin)
Comment on attachment 8548679 [details] [review]
Link to github

Looks good, thanks!
Attachment #8548679 - Flags: review?(azasypkin) → review+
In master :
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 8548679 [details] [review]
Link to github

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): N/A
[User impact] if declined: Takes a little more time to app fully loaded
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A
Attachment #8548679 - Flags: approval-gaia-v2.2?
Attachment #8548679 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Whiteboard: [p=1] → [p=1][sms-sprint-2.2S4]
Whiteboard: [p=1][sms-sprint-2.2S4] → [p=1]
You need to log in before you can comment on or make changes to this bug.