The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 17

Status

()

Core
Widget: Win32
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: (dormant account), Assigned: tnikkel)

Tracking

unspecified
mozilla18
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 fixed)

Details

(Whiteboard: [Snappy])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
with paint_flashing on I see that twitter invalidates most of the page on every keystroke
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

5 years ago
A regression range would help.
(Reporter)

Comment 4

5 years ago
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.
Blocks: 775913
(Reporter)

Comment 5

5 years ago
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
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.
(Assignee)

Comment 7

5 years ago
(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.
(Reporter)

Comment 8

5 years ago
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?
(Assignee)

Comment 9

5 years ago
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.
(Reporter)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
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.
Attachment #656755 - Flags: review?(roc)
(Assignee)

Comment 12

5 years ago
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.
Attachment #656755 - Flags: review?(roc) → review+

Comment 13

5 years ago
(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.)
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
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.
Assignee: nobody → tnikkel
Attachment #656899 - Flags: review?(jmathies)
(Assignee)

Comment 16

5 years ago
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.
Blocks: 357725
(Assignee)

Updated

5 years ago
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?

Comment 17

5 years ago
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.
Attachment #656899 - Flags: review?(jmathies) → review+
(Assignee)

Comment 18

5 years ago
Part 1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ceffe737dd
(Assignee)

Updated

5 years ago
Blocks: 574635
https://hg.mozilla.org/mozilla-central/rev/e0ceffe737dd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Re-open since part 2 is pending.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [Snappy]
Next time, be sure to put [leave open] on the whiteboard so that our merge script doesn't resolve it.
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d50d1c9192
Backed out for mochitest-2 and xpcshell test breakage

http://hg.mozilla.org/integration/mozilla-inbound/rev/d4d50d1c9192
Sorry, I misused my own bug-commenting tool. I did *not*, in fact, back out any patch from here.
https://hg.mozilla.org/mozilla-central/rev/d4d50d1c9192
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 26

5 years ago
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
Attachment #656755 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 27

5 years ago
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
Attachment #656899 - Flags: approval-mozilla-aurora?
Already fixed, and will approve uplift so clearing the tracking flags.
tracking-firefox17: ? → ---
tracking-firefox18: ? → ---
Attachment #656755 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #656899 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 29

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/7bb4bfd12256
https://hg.mozilla.org/releases/mozilla-aurora/rev/5197b5162031
status-firefox17: --- → fixed
(Assignee)

Updated

5 years ago
Depends on: 788189

Comment 30

5 years ago
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!
(Assignee)

Updated

5 years ago
Depends on: 789482
Keywords: verifyme
(Reporter)

Comment 31

5 years ago
Anthony, this fix got rid of the lag I was experiencing.
Status: RESOLVED → VERIFIED
(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?
Keywords: verifyme
(Reporter)

Comment 33

5 years ago
(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.
(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?

Updated

5 years ago
Depends on: 802456

Comment 35

5 years ago
Backout part 1 from Aurora and fixed in a different way in Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd53d0988e6f
(Assignee)

Comment 36

4 years ago
And same thing for beta
https://hg.mozilla.org/releases/mozilla-beta/rev/8778dcf8ffb0
You need to log in before you can comment on or make changes to this bug.