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)
Tracking
()
VERIFIED
FIXED
mozilla18
Tracking | Status | |
---|---|---|
firefox17 | --- | fixed |
People
(Reporter: taras.mozilla, Assigned: tnikkel)
References
Details
(Whiteboard: [Snappy])
Attachments
(2 files)
(deleted),
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jimm
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
with paint_flashing on I see that twitter invalidates most of the page on every keystroke
Comment 2•12 years ago
|
||
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•12 years ago
|
||
A regression range would help.
Reporter | ||
Comment 4•12 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.
Reporter | ||
Comment 5•12 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
Comment 6•12 years ago
|
||
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•12 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•12 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•12 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•12 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•12 years ago
|
||
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•12 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•12 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•12 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•12 years ago
|
||
Assignee: nobody → tnikkel
Attachment #656899 -
Flags: review?(jmathies)
Assignee | ||
Comment 16•12 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•12 years ago
|
tracking-firefox17:
--- → ?
tracking-firefox18:
--- → ?
Comment 17•12 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•12 years ago
|
||
Comment 19•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 20•12 years ago
|
||
Re-open since part 2 is pending.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [Snappy]
Comment 21•12 years ago
|
||
Next time, be sure to put [leave open] on the whiteboard so that our merge script doesn't resolve it.
Assignee | ||
Comment 22•12 years ago
|
||
Comment 23•12 years ago
|
||
Backed out for mochitest-2 and xpcshell test breakage
http://hg.mozilla.org/integration/mozilla-inbound/rev/d4d50d1c9192
Comment 24•12 years ago
|
||
Sorry, I misused my own bug-commenting tool. I did *not*, in fact, back out any patch from here.
Comment 25•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•12 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•12 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?
Comment 28•12 years ago
|
||
Already fixed, and will approve uplift so clearing the tracking flags.
tracking-firefox17:
? → ---
tracking-firefox18:
? → ---
Updated•12 years ago
|
Attachment #656755 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #656899 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 29•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7bb4bfd12256
https://hg.mozilla.org/releases/mozilla-aurora/rev/5197b5162031
status-firefox17:
--- → fixed
Comment 30•12 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!
Reporter | ||
Comment 31•12 years ago
|
||
Anthony, this fix got rid of the lag I was experiencing.
Status: RESOLVED → VERIFIED
Comment 32•12 years ago
|
||
(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•12 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.
Comment 34•12 years ago
|
||
(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•12 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•12 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.
Description
•