786421 - Extreme lag during text input on twitter/facebook/other web forms

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[(dormant account)]

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)]

with paint_flashing on I see that twitter invalidates most of the page on every keystroke

Comment 2

[Benoit Girard (:BenWa)]

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]]

A regression range would help.

Comment 4

[(dormant account)]

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)]

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

[(no longer active)]

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)]

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

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)]

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)]

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

Attachment: 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)]

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]]

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

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

Attachment: Part 2. Don't do so much extra work if we aren't changing the size of the window.

Comment 16

[Timothy Nikkel (:tnikkel)]

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]]

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)]

Part 1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ceffe737dd

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0ceffe737dd

Comment 20

[Benoit Girard (:BenWa)]

Re-open since part 2 is pending.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d50d1c9192

Comment 23

[Steve Fink [:sfink] [:s:]]

Backed out for mochitest-2 and xpcshell test breakage

http://hg.mozilla.org/integration/mozilla-inbound/rev/d4d50d1c9192

Comment 24

[Steve Fink [:sfink] [:s:]]

Sorry, I misused my own bug-commenting tool. I did *not*, in fact, back out any patch from here.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4d50d1c9192

Comment 26

[Timothy Nikkel (:tnikkel)]

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)]

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]

Already fixed, and will approve uplift so clearing the tracking flags.

Comment 29

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7bb4bfd12256
https://hg.mozilla.org/releases/mozilla-aurora/rev/5197b5162031

Comment 30

[bugzmnr]

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)]

Anthony, this fix got rid of the lag I was experiencing.

Comment 32

[u279076]

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

(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

[u279076]

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

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)]

And same thing for beta
https://hg.mozilla.org/releases/mozilla-beta/rev/8778dcf8ffb0