Closed
Bug 1259641
Opened 8 years ago
Closed 8 years ago
[e10s] MediaFeatureValuesChangedAllDocuments takes tons of time when resizing e10s window
Categories
(Core :: Layout, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: smaug, Assigned: wcpan)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
1021 bytes,
patch
|
Details | Diff | Splinter Review | |
58 bytes,
text/x-review-board-request
|
smaug
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
I think SizeModeChanged should be sent only to the active tab. Currently resizing window with lots of tabs is super slow (this is couple of days old Nightly). We've had similar issues before, in e10s and non-e10s in the past, but those issues were fixed. e10s used to behave fine here, so this is a regression, from bug 1104916 if I read the blame correctly.
Comment 1•8 years ago
|
||
[Tracking Requested - why for this release]: regression in Firefox 37
Comment 2•8 years ago
|
||
Is this during minimize/maximize resizing or adjusting the size of window by dragging a border? Also, how many tabs? I don't see a difference with 20 tabs on OSX.
Flags: needinfo?(bugs)
Reporter | ||
Comment 3•8 years ago
|
||
I saw this when resizing the window by dragging a border. I have usually >500 tabs, all the them have loaded some page. The question isn't about the number of tabs, but whether we calculate something for background tabs when we shouldn't need to.
Flags: needinfo?(bugs)
Comment 4•8 years ago
|
||
bug 1256084 may help as well
Assignee | ||
Comment 6•8 years ago
|
||
Ok, I'll look this one.
Assignee: nobody → wpan
Flags: needinfo?(wpan)
Why are we restyling for just resizing a window, even for the active tab? That seems extremely excessive.
Comment 8•8 years ago
|
||
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #7) > Why are we restyling for just resizing a window, even for the active tab? > That seems extremely excessive. From the code I guess it's for media queries. Do you think we could skip the restyling and layout will take care of it?
We should restyle when we actually cross the boundary of an actual media query that exists in the document's style sheets. That's what nsMediaQueryResultCacheKey is for. But we certainly shouldn't restyle on every resize.
Comment 10•8 years ago
|
||
BTW, it's from bug 1104916
Comment 11•8 years ago
|
||
When I was looking at this awhile back, windows, linux and osx all seem to have different behavior for when the sizemodechanged event is triggered. Some of the platforms guard against sending the event if the size mode hasn't really changed while others always send it even if the value is the same. If I remember correctly linux always sent the event. When changing the window size the size mode shouldn't really change, so it may be as simple as putting in a guard to not send this event unless the new value differs from the old. I'm not sure if any code relies on this behavior though. I'll be on PTO the next two weeks so I wont' be able to look into this.
Assignee | ||
Comment 12•8 years ago
|
||
Don't request reflow for background tabs, WIP. Passed on try server. I don't see any difference even if I don't reflow in TabChild::RecvSizeModeChanged for foreground tabs though. Maybe there is a tricky timing issue in e10s so we need the reflow?
We shouldn't be doing unnecessary restyles for foreground tabs either. The right approach to fixing this is probably by addressing comment 11.
Comment on attachment 8742110 [details] [diff] [review] dont-request-reflow-for-background-tabs.patch And, actually, the media query code should already be handling checking whether any restyling needs to happen by itself, if it weren't for this call, in the context of the patch: > presContext->MediaFeatureValuesChangedAllDocuments(eRestyle_Subtree, > NS_STYLE_HINT_REFLOW); For size mode changes, this should be passing nsRestyleHint(0) as the first parameter, and omitting the second parameter. These parameters should only have other values if restyling is required for all changes because the value change causes style changes in ways *other* than changing media query results, which is not the case for display-mode. That said, the checks the media query code still have some cost, so it's probably worth coalescing the size mode changes on platforms that send them excessively.
And note that the incorrect code is in 2 different places (TabChild::RecvSizeModeChanged and nsPresContext::SizeModeChanged), although maybe it shouldn't be duplicated like that (see bug 1104916 comment 35).
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47587/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47587/
Attachment #8743124 -
Flags: review?(bugs)
Assignee | ||
Comment 17•8 years ago
|
||
On Linux (gtk3), I cannot reproduce the behavior mentioned in comment 11, window resizing does not trigger nsPresContext::SizeModeChanged. Because nsPresContext::SizeModeChanged has been fixed in bug 1256084, so I simply changed TabChild::RecvSizeModeChanged to reuse nsPresContext::SizeModeChanged. From my testing (with 500 tabs) this patch makes the size mode transition faster, and I didn't see breakage on background tabs.
Reporter | ||
Comment 18•8 years ago
|
||
Comment on attachment 8743124 [details] MozReview Request: Bug 1259641 - Do not force reflow for all tabs when size mode changed. r?smaug https://reviewboard.mozilla.org/r/47587/#review44593 ::: dom/ipc/TabChild.cpp:1666 (Diff revision 1) > nsCOMPtr<nsIDocument> document(GetDocument()); > nsCOMPtr<nsIPresShell> presShell = document->GetShell(); > if (presShell) { > nsPresContext* presContext = presShell->GetPresContext(); > if (presContext) { > - presContext->MediaFeatureValuesChangedAllDocuments(eRestyle_Subtree, > + presContext->SizeModeChanged(aSizeMode); Ok, so this works because we support only 'browser' and 'fullscreen' modes, and background tabs are always in 'browser'. Could you add a comment to http://mxr.mozilla.org/mozilla-central/source/layout/style/nsMediaFeatures.cpp#316 , to the switch-case there, that if new modes are supported, better to ensure the setup works also in background tabs. (I could see us supporting also minimal-ui, for the F11 style fullscreen.) This could be optimized further by comparing whether the aSizeMode ends up changing display mode, but that could be done later if needed.
Attachment #8743124 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8743124 [details] MozReview Request: Bug 1259641 - Do not force reflow for all tabs when size mode changed. r?smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47587/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 20•8 years ago
|
||
Filed a follow-up: bug 1266660.
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4965b4f20933
Keywords: checkin-needed
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4965b4f20933
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Hi Jim, Wei-Cheng, this is a P1 regression in e10s. Is this severe enough (given e10s experiment) to uplift to Beta47? If not, I'd prefer to not uplift and wontfix for Fx47. Please let me know.
Flags: needinfo?(wpan)
Flags: needinfo?(jmathies)
This should get uplifted. The patch affects e10s only. And given that we use the e10s code in some cases, I'm frankly not comfortable with the risk of performance regression without taking it, unless we back out bug 1104916 for 47.
Comment 25•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #23) > Hi Jim, Wei-Cheng, this is a P1 regression in e10s. Is this severe enough > (given e10s experiment) to uplift to Beta47? If not, I'd prefer to not > uplift and wontfix for Fx47. Please let me know. No regressions for a couple weeks. This didn't help or hurt any of our release criteria but based on dbaron's comment sounds like it help with window resizing perf. I'd say we should take it. Wei-Cheng, can you please flag for uplift up to beta?
Flags: needinfo?(jmathies)
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8743124 [details] MozReview Request: Bug 1259641 - Do not force reflow for all tabs when size mode changed. r?smaug Approval Request Comment [Feature/regressing bug #]: bug 1104916. [User impact if declined]: Performance of resizing window will be bad for e10s users. [Describe test coverage new/current, TreeHerder]: Already cooked in m-c for awhile. [Risks and why]: This does not break things in 47, and do improve window resizing perf. [String/UUID change made/needed]: None.
Flags: needinfo?(wpan)
Attachment #8743124 -
Flags: approval-mozilla-aurora?
Comment on attachment 8743124 [details] MozReview Request: Bug 1259641 - Do not force reflow for all tabs when size mode changed. r?smaug This is already fixed on 48, which is now Aurora. The approval request should be for Beta.
Attachment #8743124 -
Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Thanks DBaron, Jimm for a quick reply and I'd be happy to uplift.
status-firefox47:
--- → affected
Comment on attachment 8743124 [details] MozReview Request: Bug 1259641 - Do not force reflow for all tabs when size mode changed. r?smaug Improves windows resizing performance in e10s, Beta47+
Attachment #8743124 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 30•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5aa970a65440
You need to log in
before you can comment on or make changes to this bug.
Description
•