Closed Bug 786421 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox17 --- fixed

People

(Reporter: taras.mozilla, Assigned: tnikkel)

References

Details

(Whiteboard: [Snappy])

Attachments

(2 files)

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.
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.
A regression range would help.
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.
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.
(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.
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?
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.
(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.
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)
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.
(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.)
(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: nobody → tnikkel
Attachment #656899 - Flags: review?(jmathies)
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
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+
Blocks: 574635
Status: NEW → RESOLVED
Closed: 12 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.
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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?
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.
Attachment #656755 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #656899 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 788189
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!
Depends on: 789482
Keywords: verifyme
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
(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?
Depends on: 802456
Backout part 1 from Aurora and fixed in a different way in Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/dd53d0988e6f
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: