If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[sms] Do less synchronous reflows for the compose input height management

RESOLVED WORKSFORME

Status

Firefox OS
Gaia::SMS
P4
normal
RESOLVED WORKSFORME
4 years ago
2 years ago

People

(Reporter: julienw, Unassigned)

Tracking

({perf})

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [c=effect p= s= u=])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
In the 2 methods "updateInputHeight" and "setInputMaxHeight" we're doing lots of calculations, and some of them are (probably) triggering synchronous reflows.

This bug is to investigate if we're really doing some and to fix them if possible, possibly inserting some geneated CSS style sheets.

The user-facing effect is an effect of lag while he's typing a SMS, or when he's displaying/hiding the keyboard, which is extremely unpleasant.
(Reporter)

Updated

4 years ago
Depends on: 902500
ni? Julien. please renom after further investigation on how much impact it is to the end users. Thanks
blocking-b2g: koi? → ---
Flags: needinfo?(felash)
(Reporter)

Comment 2

4 years ago
You should try writing a SMS with a loaded thread (try one of the reference workload, even the light one) to know the user impact. You can also talk with Alexandre Lissy ;)

It's _very_ unpleasant.

The reflow time when showing/hiding the keyboard is also very long, and profiling with Gabriele we saw there were 3 synchronous workflows. This is most certainly avoidable.

We already started some work (see bug 902500) and we may nominate individual smaller bugs instead of this one if you do prefer.
blocking-b2g: --- → koi?
Flags: needinfo?(felash)
Blocks: 891754
blocking-b2g: koi? → koi+
Whiteboard: [u=commsapps-user c=messaging p=0]
Julien and Gabriele, could you please share the profiling you've performed ?
Flags: needinfo?(gsvelto)
Flags: needinfo?(felash)
(Reporter)

Comment 4

4 years ago
The profiling we once did is no longer relevant.

So, here are the various cases that should be profiled:
* what happens when we press the first letter in the composer
* what happens when we press the second letter
* what happens when we press the letter that makes the input grows
* what happens when we open the keyboard
* what happens when we close the keyboard

I think the most important issues are with the last 3 cases now, when we actually change measurements.
Flags: needinfo?(gsvelto)
Flags: needinfo?(felash)
Created attachment 803744 [details]
sms_profiling.zip

Please find a set of profiling data that should cover the proposed cases.
Hm sorry, covered cases should be: opening the keyboard, closing the keyboard and growing input.
(Reporter)

Comment 7

4 years ago
Alexandre> could you try to analyze them ? Or would you like Gabriele's help ? :)
When loading some of the Messages.sym files in Cleopatra, I see sync reflows, is it what we are trying to kill?
(In reply to Julien Wajsberg [:julienw] from comment #7)
> Alexandre> could you try to analyze them ? Or would you like Gabriele's help
> ? :)

I think Gabriele could help me understanding more precisely yes, if he has time :)
(In reply to Alexandre LISSY :gerard-majax (off 26/08-08/09) from comment #9)
> I think Gabriele could help me understanding more precisely yes, if he has
> time :)

I've got a last patch to finish and then I'll come over to help :)
(Reporter)

Comment 11

4 years ago
This is not a release blocker, we already did some of it for 1.2, let's move the rest to 1.3.
blocking-b2g: koi+ → 1.3?
Not blocking but we would definitely take a patch
minused per comment 12
blocking-b2g: 1.3? → -
(Reporter)

Updated

4 years ago
Depends on: 949457
Blocks: 888135

Updated

4 years ago
Keywords: perf
blocking-b2g: - → backlog

Comment 14

3 years ago
Julien, is this still an issue?
Flags: needinfo?(felash)
Priority: -- → P4
Whiteboard: [u=commsapps-user c=messaging p=0] → [c=effect p= s= u=]
(Reporter)

Comment 15

3 years ago
I think bug 949457 fixed this now but I'd like a profile before closing it.
Flags: needinfo?(felash)
(Assignee)

Updated

3 years ago
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
(Reporter)

Updated

3 years ago
Summary: [sms] Do less synchronous workflows for the compose input height management → [sms] Do less synchronous reflows for the compose input height management
(Reporter)

Comment 16

2 years ago
The reflows (layout + paint especially) are not synchronous anymore.

They're still quite costly though: layout takes 50ms and paint 30ms on my workload, which is expected because it happens in a big thread.
In the "New message" window, so with a much smaller DOM, layout is a lot shorter (15ms) but paint is still quite long (can take ~27ms). I guess this is costly because we're painting the whole contenteditable at each keypress... 
I'll file a separate bug with a smaller testcase.
Flags: needinfo?(felash)
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 17

2 years ago
added a testcase in bug 972238.
Flags: needinfo?(felash)
You need to log in before you can comment on or make changes to this bug.