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

Status: RESOLVED WORKSFORME

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

Description

[Julien Wajsberg [:julienw] (PTO -> may 13)]

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.

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

ni? Julien. please renom after further investigation on how much impact it is to the end users. Thanks

Comment 2

[Julien Wajsberg [:julienw] (PTO -> may 13)]

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.

Comment 3

[:gerard-majax]

Julien and Gabriele, could you please share the profiling you've performed ?

Comment 4

[Julien Wajsberg [:julienw] (PTO -> may 13)]

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.

Comment 5

[:gerard-majax]

Attachment: sms_profiling.zip

Please find a set of profiling data that should cover the proposed cases.

Comment 6

[:gerard-majax]

Hm sorry, covered cases should be: opening the keyboard, closing the keyboard and growing input.

Comment 7

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Alexandre> could you try to analyze them ? Or would you like Gabriele's help ? :)

Comment 8

[:gerard-majax]

When loading some of the Messages.sym files in Cleopatra, I see sync reflows, is it what we are trying to kill?

Comment 9

[:gerard-majax]

(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 :)

Comment 10

[Gabriele Svelto [:gsvelto]]

(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 :)

Comment 11

[Julien Wajsberg [:julienw] (PTO -> may 13)]

This is not a release blocker, we already did some of it for 1.2, let's move the rest to 1.3.

Comment 12

[David Scravaglieri [:scravag]]

Not blocking but we would definitely take a patch

Comment 13

[David Scravaglieri [:scravag]]

minused per comment 12

Comment 14

[Mike Lee [:mlee]]

Julien, is this still an issue?

Comment 15

[Julien Wajsberg [:julienw] (PTO -> may 13)]

I think bug 949457 fixed this now but I'd like a profile before closing it.

Comment 16

[Julien Wajsberg [:julienw] (PTO -> may 13)]

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.

Comment 17

[Julien Wajsberg [:julienw] (PTO -> may 13)]

added a testcase in bug 972238.