1373023 - [meta] Laggy text input on Twitter direct messages (we flush layout too often, and reflow is slow)

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Andrew Overholt [:overholt]]

While in a conversation on Twitter with paint flashing on, I see a lot of painting areas earlier in the conversation when typing. After a number of messages back and forth (maybe 15?), input latency (i.e. letters I type showing up on the screen) gets bad; maybe this is related?

Comment 1

[Andrew Overholt [:overholt]]

Ehsan noticed Twitter doing some sort of updating while we were simulating a conversation so maybe it's not entirely Gecko's fault.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Did you get a profile for this?

Comment 3

[Andrew Overholt [:overholt]]

Ehsan did.

Comment 4

[(no longer active)]

I lost the link to the profile unfortunately, but it wasn't showing anything interesting.  Although I captured yet another profile later that did show a jank but unfortunately the updater had switched up my build under the hood and the symbols were corrupted so the profile was unreadable.  I suggest someone should spend some time reprofiling, but it looked like there was some expensive synchronous layout flush involved.

Comment 5

[Andrew Overholt [:overholt]]

Here's a profile I got just now: http://bit.ly/2sBRXNT. Ehsan said it's slow reflow with flexbox and tables.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Hmm, but we're flushing when handling some IME stuff. Could we not flush there?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Though, reflow is so super slow that any layout flush at any point would make the input handling janky.

Comment 8

[(no longer active)]

http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/editor/libeditor/HTMLAnonymousNodeEditor.cpp#319 could probably be changed to keep track of whether the table editing / absolute position editing UI is currently displayed for the editor instead and avoid flushing if it isn't.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Hmm, but we're flushing when handling some IME stuff. Could we not flush
> there?
(In reply to Olli Pettay [:smaug] from comment #7)
> Though, reflow is so super slow that any layout flush at any point would
> make the input handling janky.

When some text is inputted, we need to notify main process of new text, selection range and character position around new selection. For computing the character positions, ContentEventHandler needs to flush pending layout (painting is not necessary, though, reflow is needed). If we do it a lot during an input event handling, we should stop doing it except the last one. However, if it's not, I have no idea. If we put off it, IME may show its windows, e.g., navigation window, suggest window and candidate window, at odd position. That makes users feel buggy.

The worst scenario is, web apps modify DOM nodes after every text input asynchronously. Then, at least two reflow are required.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I wonder how bad it would be to notify parent process once per animation frame tick?
In the profile is looks like we end up handling two or three input events between refresh driver ticks and all them do slow flush.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #10)
> I wonder how bad it would be to notify parent process once per animation
> frame tick?
> In the profile is looks like we end up handling two or three input events
> between refresh driver ticks and all them do slow flush.

Honestly, not sure. I think that IMM, TSF and GTK are fine because IME windows are adjust with every new notification. But on macOS, we cannot notify IME of layout change. On the other hand, in e10s mode, we already notify requested character position before handling focused remote process. So, I guess that it's no problem.

The trigger of kicking ContentEventHandler is IMEContentObserver::TryToFlushPendingNotifications() which is called by PresShell::HandleEventInternal() after dispatching a DOM event.
https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla18IMEContentObserver30TryToFlushPendingNotificationsEv&redirect=false

I don't know how to call it from refresh driver, though.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: this show 4 places where we flush and shouldn't if possible.

This is just showing were we flush and shouldn't.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, but we cannot remove the call of IMEContentObserver::TryToFlushPendingNotifications() in PresShell::HandleEventInternal() if it's in the main process because if it's in chrome process, we don't use ContentCache. Therefore, any query events will fail instead of return dummy (cached) values.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

We should basically try to move all the flushing here to happen around tick time, I think, but
ensure that IME handling doesn't regress.
Would that make sense from IME point of view?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

That patch is just a simple thing showing where the issues are, not any proposal how they should be fixed.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #15)
> We should basically try to move all the flushing here to happen around tick
> time, I think, but
> ensure that IME handling doesn't regress.
> Would that make sense from IME point of view?

Yes, of course.

But not using ContentCache when main process content has focus may be a problem. That means that we may need to different handling if it's in main process or content process. If we could use ContentCache even in main process, IME related code won't need to check if it's in main process. But I'm still not sure, where should ContentCache be.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

In other words, when main process's content has focus, IME handling in widget still depends on synchronous content update for every input event.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

That is probably fine. We could special case child process handling to be async.
(the patch shows also an issue in editor and in selection handling)

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Filed bugs about the flushes, reflow part needs to be still analyzed.

Comment 21

[Andrew Overholt [:overholt]]

Marking [qf] because smaug thinks this is indicative of general input latency issues.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Once bug 1375491 is in Nightlies, could you Andrew retest, and possibly get a new profile if there are still issues.

Comment 23

[Daniel Holbert [:dholbert]]

I spun off bug 1377253 for improving the flex layout time here.

Comment 24

[Andrew Overholt [:overholt]]

(In reply to Andrew Overholt [:overholt] from comment #21)
> Marking [qf] because smaug thinks this is indicative of general input
> latency issues.

it seems much better now!

Comment 25

[Daniel Holbert [:dholbert]]

I think we can call this fixed -- bug 1375491 fixed most of it (reducing the number of reflows), and per bug 1377253 comment 11, the reflows that we are doing seem to be pretty short now.

If anyone's still seeing layout-related jank when typing in a twitter Direct Message, though, then please reopen bug 1377253 (ideally with a perf-html.io performance profile and info that might help with reproducing, e.g. whether backscroll/emoji/images/whatever seem to be related.)