Last Comment Bug 786421 - Extreme lag during text input on twitter/facebook/other web forms
: Extreme lag during text input on twitter/facebook/other web forms
Status: VERIFIED FIXED
[Snappy]
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla18
Assigned To: Timothy Nikkel (:tnikkel)
:
Mentors:
Depends on: 788189 789482 802456
Blocks: 357725 574635 775913
  Show dependency treegraph
 
Reported: 2012-08-28 12:53 PDT by (dormant account)
Modified: 2012-11-06 14:39 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Part 1. Don't size the view to 0,0. (1.17 KB, patch)
2012-08-29 22:46 PDT, Timothy Nikkel (:tnikkel)
roc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 2. Don't do so much extra work if we aren't changing the size of the window. (1.89 KB, patch)
2012-08-30 09:00 PDT, Timothy Nikkel (:tnikkel)
jmathies: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description (dormant account) 2012-08-28 12:53:15 PDT
Facebook typing lag 
http://people.mozilla.com/~bgirard/cleopatra/?report=5542a2ce52471e04835771e84c78b8db2681731a

Twitter typing lag
http://people.mozilla.com/~bgirard/cleopatra/?report=f4a54e7a57de3570818ade540531300e97d133e0

About 17-20% of cpu time while typing is spent on nsBaseClient::ResizeClient even though there is no resizing going on. Within that, 10% of time is spent on nsWindow::ClearThemeRegion.

Search for "Resize" in the profiles to see the interesting stacks.
Comment 1 (dormant account) 2012-08-28 12:57:08 PDT
with paint_flashing on I see that twitter invalidates most of the page on every keystroke
Comment 2 Benoit Girard (:BenWa) 2012-08-28 14:58:51 PDT
Any idea what is going on? Let me know if you have trouble reading the profiles. 

Basically script causes layout flush, which resizes a windows, which calls NtUserSetWindowRgn and the likes.
Comment 3 Jim Mathies [:jimm] 2012-08-28 15:14:05 PDT
A regression range would help.
Comment 4 (dormant account) 2012-08-28 15:22:40 PDT
Note that a similar problem occurs while idling(and blinking the caret).

http://people.mozilla.com/~bgirard/cleopatra/?report=3cc3f3b1fbbec7533154c5190768ad74bf419505

~70% of the overhead is in the bogus resize.
Comment 5 (dormant account) 2012-08-28 15:47:44 PDT
Jim suggested this might be a fallout from bug 743975. I tried a nightly from 08-15. It's still doing pointless Resizes + theme stuff while blinking caret. The overhead of resize portion of the redraw looks to be slightly lower(50% vs 70%, but that might be a fluke).

http://people.mozilla.com/~bgirard/cleopatra/?report=2faf6359d423de45b8408fd753f4b06e18d711af
Comment 6 :Ehsan Akhgari 2012-08-28 16:36:55 PDT
What happens under the flushes is sort of irrelevant since that's just delayed work.  Someone should set a breakpoint on nsView::ResetWidgetBounds to see what calls that function.  That is the cause of the async resize events which are processed when flushing.
Comment 7 Timothy Nikkel (:tnikkel) 2012-08-29 17:28:01 PDT
(In reply to Taras Glek (:taras) from comment #1)
> with paint_flashing on I see that twitter invalidates most of the page on
> every keystroke

I think this is a separate issue. The twitter site has changed and regressed this.
Comment 8 (dormant account) 2012-08-29 17:30:55 PDT
I made a custom build, added a printf 
http://mxr.mozilla.org/mozilla-central/source/view/src/nsView.cpp#278

It seems that at first everything behave fine, but after some browser activity
Firefox is constantly resizing something, it's like a flag isn't getting reset somewhere.

The if (!changedPos) branch keeps processing the same resize request over and over:
eg: curBounds->newBounds
 [9,22]->[0,0]

Any ideas?
Comment 9 Timothy Nikkel (:tnikkel) 2012-08-29 17:34:34 PDT
In nsMenuPopupFrame::HidePopup we resize the view for the popup to 0,0,0,0, but we have a size constraint set on the widget that prevents it from being sized this small. So each time we walk the view tree making sure the sizes of widgets are up to date we think that hidden popups have the wrong size and we try to resize them. The resize to 0,0 gets forced to the min size from the previously set size constraints. Due to the way the widget resize code is written we end up doing a lot of extra work. All the hidden popups (menus, tooltips, etc) have to do this useless work.
Comment 10 (dormant account) 2012-08-29 17:49:34 PDT
(In reply to Timothy Nikkel (:tn) from comment #9)
> In nsMenuPopupFrame::HidePopup we resize the view for the popup to 0,0,0,0,
> but we have a size constraint set on the widget that prevents it from being
> sized this small. So each time we walk the view tree making sure the sizes
> of widgets are up to date we think that hidden popups have the wrong size
> and we try to resize them. The resize to 0,0 gets forced to the min size
> from the previously set size constraints. Due to the way the widget resize
> code is written we end up doing a lot of extra work. All the hidden popups
> (menus, tooltips, etc) have to do this useless work.

Is this a regression?

Is it also a bug that we try to theme widgets in nsWindow::Resize even though their requested size is 0? Heavy weight windows themeing code is the only reason i noticed this.
Comment 11 Timothy Nikkel (:tnikkel) 2012-08-29 22:46:11 PDT
Created attachment 656755 [details] [diff] [review]
Part 1. Don't size the view to 0,0.

I don't think we need to size the view to 0,0. The origin of this line is one of those changesets from 2000 with no bug number and a vague checkin comment. The only thing I can think of that this might affect is invalidates in hidden popups: they would usually get ignored because they become zero sized but now they might get to the widget level. The view and widget is hidden so I don't think this matters.
Comment 12 Timothy Nikkel (:tnikkel) 2012-08-29 22:54:19 PDT
Jim, you changed it so we do a full Resize call when aRepaint is true even if the bounds are the same in bug 574635. Do you remember why we wanted that change?

I think the other backends just treat aRepaint as asking for the changed area to be repainted, not to repainting the window no matter what. It would be nice if we could avoid all this extra work and the full window repaint if we don't need to do any size change.
Comment 13 Jim Mathies [:jimm] 2012-08-30 01:10:35 PDT
(In reply to Timothy Nikkel (:tn) from comment #12)
> Jim, you changed it so we do a full Resize call when aRepaint is true even
> if the bounds are the same in bug 574635. Do you remember why we wanted that
> change?
> 
> I think the other backends just treat aRepaint as asking for the changed
> area to be repainted, not to repainting the window no matter what. It would
> be nice if we could avoid all this extra work and the full window repaint if
> we don't need to do any size change.

I really don't remember. We can try ignoring that repaint parameter again and see what crops up.  (If we do we should update the nsIWidget commenting on all the resize calls.)
Comment 14 Timothy Nikkel (:tnikkel) 2012-08-30 08:59:52 PDT
(In reply to Jim Mathies [:jimm] from comment #13)
> I really don't remember. We can try ignoring that repaint parameter again
> and see what crops up.  (If we do we should update the nsIWidget commenting
> on all the resize calls.)

I have a patch that skips all the rest of the stuff but still does the repaint, which shouldn't be that big of a deal since the resulting paint will just composite retained content to the screen. And it will be an improvement.
Comment 15 Timothy Nikkel (:tnikkel) 2012-08-30 09:00:36 PDT
Created attachment 656899 [details] [diff] [review]
Part 2. Don't do so much extra work if we aren't changing the size of the window.
Comment 16 Timothy Nikkel (:tnikkel) 2012-08-30 09:33:42 PDT
The size constraint stuff was added by http://hg.mozilla.org/mozilla-central/rev/b8a0228a10fa from bug 357725. So this is probably caused by that.
Comment 17 Jim Mathies [:jimm] 2012-08-30 09:47:11 PDT
Comment on attachment 656899 [details] [diff] [review]
Part 2. Don't do so much extra work if we aren't changing the size of the window.

Looks ok to me.
Comment 18 Timothy Nikkel (:tnikkel) 2012-08-30 09:49:25 PDT
Part 1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ceffe737dd
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-08-30 19:03:09 PDT
https://hg.mozilla.org/mozilla-central/rev/e0ceffe737dd
Comment 20 Benoit Girard (:BenWa) 2012-08-30 19:16:09 PDT
Re-open since part 2 is pending.
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-08-30 19:18:39 PDT
Next time, be sure to put [leave open] on the whiteboard so that our merge script doesn't resolve it.
Comment 22 Timothy Nikkel (:tnikkel) 2012-08-31 09:17:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d50d1c9192
Comment 23 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-08-31 09:21:16 PDT
Backed out for mochitest-2 and xpcshell test breakage

http://hg.mozilla.org/integration/mozilla-inbound/rev/d4d50d1c9192
Comment 24 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-08-31 09:26:39 PDT
Sorry, I misused my own bug-commenting tool. I did *not*, in fact, back out any patch from here.
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-08-31 18:45:01 PDT
https://hg.mozilla.org/mozilla-central/rev/d4d50d1c9192
Comment 26 Timothy Nikkel (:tnikkel) 2012-09-04 09:23:10 PDT
Comment on attachment 656755 [details] [diff] [review]
Part 1. Don't size the view to 0,0.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 357725
User impact if declined: extra jank when doing anything that draws to the screen
Testing completed (on m-c, etc.): on nightly for several days
Risk to taking this patch (and alternatives if risky): should be pretty safe
String or UUID changes made by this patch: none
Comment 27 Timothy Nikkel (:tnikkel) 2012-09-04 09:23:26 PDT
Comment on attachment 656899 [details] [diff] [review]
Part 2. Don't do so much extra work if we aren't changing the size of the window.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 357725
User impact if declined: extra jank when doing anything that draws to the screen
Testing completed (on m-c, etc.): on nightly for several days
Risk to taking this patch (and alternatives if risky): should be pretty safe
String or UUID changes made by this patch: none
Comment 28 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-04 16:43:38 PDT
Already fixed, and will approve uplift so clearing the tracking flags.
Comment 30 bugzmnr 2012-09-07 14:26:30 PDT
Was the jank of this bug internal to Firefox, or did it interact with the OS WM in any way? (i.e. by repeatedly setting the properties of a window or something)

The Nightlies got unbearably unusable for me under Remote Desktop with WDM remoting (which sends the top-level windows individually to the client, as opposed to the composited desktop). The problem magically disappeared later on, then I read about this in Taras' Snappy #38 blog post.

By the looks of it this might have been the cause. In either case, thank you!
Comment 31 (dormant account) 2012-09-20 00:32:27 PDT
Anthony, this fix got rid of the lag I was experiencing.
Comment 32 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-20 09:02:30 PDT
(In reply to Taras Glek (:taras) from comment #31)
> Anthony, this fix got rid of the lag I was experiencing.

Thanks Taras. Is it safe to mark this verified for Firefox 17 as well?
Comment 33 (dormant account) 2012-09-24 16:06:51 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #32)
> (In reply to Taras Glek (:taras) from comment #31)
> > Anthony, this fix got rid of the lag I was experiencing.
> 
> Thanks Taras. Is it safe to mark this verified for Firefox 17 as well?

Think so. I only tested on 18.
Comment 34 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-27 14:39:05 PDT
(In reply to Taras Glek (:taras) from comment #33)
> Think so. I only tested on 18.

Would you be able to test against Firefox 17, just to be thorough?
Comment 35 Scoobidiver (away) 2012-11-04 01:06:12 PDT
Backout part 1 from Aurora and fixed in a different way in Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd53d0988e6f
Comment 36 Timothy Nikkel (:tnikkel) 2012-11-06 14:39:41 PST
And same thing for beta
https://hg.mozilla.org/releases/mozilla-beta/rev/8778dcf8ffb0

Note You need to log in before you can comment on or make changes to this bug.