Closed Bug 1259641 Opened 4 years ago Closed 4 years ago

[e10s] MediaFeatureValuesChangedAllDocuments takes tons of time when resizing e10s window

Categories

(Core :: Layout, defect, P1)

36 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox47 + fixed
firefox48 --- fixed

People

(Reporter: smaug, Assigned: wcpan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
[Tracking Requested - why for this release]: regression in Firefox 37
Blocks: e10s-perf
tracking-e10s: --- → +
Priority: -- → P1
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)
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)
bug 1256084 may help as well
Wei-Cheng, could you take a look?
Flags: needinfo?(wpan)
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.
(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.
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.
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).
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.
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+
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/
https://hg.mozilla.org/mozilla-central/rev/4965b4f20933
Status: NEW → RESOLVED
Closed: 4 years ago
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.
(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)
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.
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+
You need to log in before you can comment on or make changes to this bug.