Closed
Bug 1373023
Opened 7 years ago
Closed 6 years ago
[meta] Laggy text input on Twitter direct messages (we flush layout too often, and reflow is slow)
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
RESOLVED
FIXED
Performance Impact | ? |
People
(Reporter: overholt, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: meta, perf)
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•7 years ago
|
||
Ehsan noticed Twitter doing some sort of updating while we were simulating a conversation so maybe it's not entirely Gecko's fault.
Comment 4•7 years ago
|
||
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.
Flags: needinfo?(ehsan)
Reporter | ||
Comment 5•7 years ago
|
||
Here's a profile I got just now: http://bit.ly/2sBRXNT. Ehsan said it's slow reflow with flexbox and tables.
Reporter | ||
Updated•7 years ago
|
Blocks: FastReflows
Component: Layout: Web Painting → Layout
Summary: Overpainting while typing in Twitter direct messages → Laggy text input on Twitter direct messages
Updated•7 years ago
|
Flags: needinfo?(dholbert)
Flags: needinfo?(dbaron)
Comment 6•7 years ago
|
||
Hmm, but we're flushing when handling some IME stuff. Could we not flush there?
Flags: needinfo?(masayuki)
Comment 7•7 years ago
|
||
Though, reflow is so super slow that any layout flush at any point would make the input handling janky.
Comment 8•7 years ago
|
||
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•7 years ago
|
||
(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.
Flags: needinfo?(masayuki)
Comment 10•7 years ago
|
||
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.
Flags: needinfo?(masayuki)
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
(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.
Flags: needinfo?(masayuki)
Comment 13•7 years ago
|
||
This is just showing were we flush and shouldn't.
Updated•7 years ago
|
Attachment #8880349 -
Attachment description: this show 4 places were we flush and shouldn't if possible. → this show 4 places where we flush and shouldn't if possible.
Comment 14•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
That patch is just a simple thing showing where the issues are, not any proposal how they should be fixed.
Comment 17•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
Filed bugs about the flushes, reflow part needs to be still analyzed.
Summary: Laggy text input on Twitter direct messages → [meta] Laggy text input on Twitter direct messages (we flush layout too often, and reflow is slow)
Reporter | ||
Comment 21•7 years ago
|
||
Marking [qf] because smaug thinks this is indicative of general input latency issues.
Whiteboard: [qf]
Updated•7 years ago
|
Whiteboard: [qf] → [qf:meta]
Comment 22•7 years ago
|
||
Once bug 1375491 is in Nightlies, could you Andrew retest, and possibly get a new profile if there are still issues.
Flags: needinfo?(overholt)
Comment 23•7 years ago
|
||
I spun off bug 1377253 for improving the flex layout time here.
Flags: needinfo?(dholbert)
Reporter | ||
Comment 24•7 years ago
|
||
(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!
Flags: needinfo?(overholt)
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Comment 25•6 years ago
|
||
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.)
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(dbaron)
Resolution: --- → FIXED
Updated•3 years ago
|
Performance Impact: --- → ?
Whiteboard: [qf:meta]
You need to log in
before you can comment on or make changes to this bug.
Description
•