Closed Bug 1186662 Opened 4 years ago Closed 4 years ago

DisplayPort should be suppressed to the viewport during sync paint operation (Resize, Tab Switch)

Categories

(Core :: Panning and Zooming, defect)

39 Branch
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

We should temporarily shrink the DisplayPort while performing syncrounous activity like scrolling using the scrollbar, resizing the browser and tab switch. Right now on desktop we're delaying sync operation to paint content that isn't visible.

I'm surprise that we didn't get major Talos regression when turning this on TBH.
Have you done any measurements/logging to see if this is actually a problem? Or is this just a hypothetical concern? I agree these are things we should do but I'd like to confirm the current behaviour and maybe measure impact before we actually do anything here.
I totally agree, we shouldn't accept complexity without knowing that it measurably helps us. I'm happy to block this bug on having this data. I don't have any measurement so it's just hypothetical at this point. Safari is doing this for resize so I suspect it's going to help.

My gut feeling is we're going to regress TResize without this.
Depends on: 1187081
I added pageload in here. It's likely ATM the only sync operation that Talos would measure properly using the tp5o pageset.
Summary: DisplayPort should shrink to the viewport during sync paint operation (Resize, Tab Switch, Scrollbar) → DisplayPort should shrink to the viewport during sync paint operation (Resize, Tab Switch, Scrollbar, PageLoad)
I started looking at the code for this but I don't really know where to track that we're in the middle of a sync operation. This is something that needs to be tracked at the window level I believe since most sync operation pertain to windows (resize, tab switch, ~scrollbar, ~pageload). Perhaps I need to track this in the outer window?

I want to be able to query this during nsLayoutUtils::GetDisplayPort. For e10s we will also need to forward this property. Maybe this needs to go in PBrowser for e10s?

Any idea kats?
Flags: needinfo?(bugmail.mozilla)
Attached patch Experiment (obsolete) — Splinter Review
Here's my failed attempt so far.
Attachment #8643360 - Attachment is patch: true
Attachment #8643360 - Attachment mime type: text/x-patch → text/plain
Assignee: nobody → bgirard
To be honest I don't know how those operations are triggered. I know how resize is triggered but tab switch, scrollbar/pageload stuff I have no idea.

That being said, I think it might be worth doing a try push where you just unconditionally disable the displayport margins and see how that affects talos numbers. If the talos regressions remain then we know we are looking in the wrong place (at least for those talos tests).
Flags: needinfo?(bugmail.mozilla)
It's not the triggering that I'm having problems with. It's how to set/get that we're in a 'sync' operation.
Ah in that case yeah sending a message from the parent process over PBrowser is probably the way to go. I would prefer some sort of RAII class in the parent process that's on the stack during the sync operation. On creation it sends a message to the child to suppress displayport usage, and on destruction it sends a message to unsuppress.
I'll try and use RAII where possible. With the resize notifications for instance I don't think RAII is possible but I'll see.
The scores show a clear improvement with no display port. I ran by patching 'GetDisplayPort' to false. Are you convinced kats by this data?
Flags: needinfo?(bugmail.mozilla)
Yes, thank you!
Flags: needinfo?(bugmail.mozilla)
Attached file tps with v1
Attached patch patch v1 (WIP) (obsolete) — Splinter Review
Attachment #8643360 - Attachment is obsolete: true
Bug 1186662 - Part 1: Prioritize DisplayPort painting during tab switch. r?kats
Attachment #8647227 - Flags: review?(bugmail.mozilla)
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

https://reviewboard.mozilla.org/r/15929/#review14269

Overall this looks fine - mostly I don't like the naming (see below) but functionally the code looks ok.

::: browser/base/content/tabbrowser.xml:3077
(Diff revision 1)
> +            // Active prioritizeViewport

This comment is not very useful. What are the keys/values in the map?

::: browser/base/content/tabbrowser.xml:3141
(Diff revision 1)
> +              this.activePrioritizeViewport.forEach(function(value, key) {

I'm not so familiar with this code so I'm not sure when this runs, but it seems like it won't get run until the browser is actually shut down. If so - that's fine for cleanup, but we need need to clear this after the tab switch is complete as well.

Somebody who's a peer of this code should review this also.

::: gfx/layers/apz/util/APZCCallbackHelper.cpp:816
(Diff revision 1)
> +APZCCallbackHelper::PrioritizeViewport(const bool& aEnabled)

Personally I don't like the naming of this. "Viewport" is a very overloaded term throughout the codebase and I would like to avoid using it here, and it's not obvious that "PrioritizeViewport" refers to painting priority. I would much prefer "displayport suppression" as the name of this feature because the displayport is much less ambigiuous than viewport, and it's more obvious that suppressing the displayport has the effect of reducing painted area.

With this in mind I would prefer to rename this function to "SuppressDisplayport(bool)" , and rename "IsPrioritizeViewport" to "IsDisplayportSuppressed". Other things should be similarly renamed.
Attachment #8647227 - Flags: review?(bugmail.mozilla)
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Part 1: Prioritize DisplayPort painting during tab switch. r?kats
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Part 1: Prioritize DisplayPort painting during tab switch. r?kats
Attachment #8647227 - Flags: review?(mconley)
Attachment #8647227 - Flags: review?(bugmail.mozilla)
Sounds good. I like SuppressDisplayport better. I wanted to get an initial review from you first in case you wanted a major change since there was other ways we could do this by going through the APZ logic instead to request a new paint.

The code I'm changing in tabbrowser.xml is only the tab browser. It's created and destroyed during tab transition only and matches the tab switch timing quite effectively.
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

https://reviewboard.mozilla.org/r/15929/#review14281

::: browser/base/content/tabbrowser.xml:3077
(Diff revision 2)
> +            // Keep an exact list of processes in which we're actively

s/processes/tabs/. Note that in the default e10s configuration all the tabs are in the same child process. (Also this means that suppressing displayport on one TabParent will actually do it for all TabParents, but I think that's ok)
Attachment #8647227 - Flags: review?(bugmail.mozilla) → review+
Oh also in the commit message s/Prioritize/Suppress/
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> Comment on attachment 8647227 [details]
> MozReview Request: Bug 1186662 - Part 1: Prioritize DisplayPort painting
> during tab switch. r?kats
> 
> https://reviewboard.mozilla.org/r/15929/#review14281
> 
> ::: browser/base/content/tabbrowser.xml:3077
> (Diff revision 2)
> > +            // Keep an exact list of processes in which we're actively
> 
> s/processes/tabs/. Note that in the default e10s configuration all the tabs
> are in the same child process. (Also this means that suppressing displayport
> on one TabParent will actually do it for all TabParents, but I think that's
> ok)

Yes, thanks for pointing that out. AFAIK the only downside is sending a few more IPC message in the single content process case but it's simpler that way. I think it's a decent trade off.
Attachment #8647227 - Attachment description: MozReview Request: Bug 1186662 - Part 1: Prioritize DisplayPort painting during tab switch. r?kats → MozReview Request: Bug 1186662 - Part 1: Add SuppressDisplayport painting and use it during tab switch. r=kats,mconley
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Part 1: Add SuppressDisplayport painting and use it during tab switch. r=kats,mconley
Working on part 2: Handling resize of mac. I'd imagine, if TResize is setup correctly, it will help with the score there as well.

Mstange, I added the viewWillStartLiveResize stuff and I’m getting the right events. Now from nsChildView I want to talk to the child content process of the current tab to send suppressViewport(true/false), I probably want to go through tabbrowser.xml to find the current selected tab. I’m wondering if I should use an observable or follow references to locate it. Looks like the window mostly talks via Events. I could also add gecko resizestart/resizeend events as well.

Can you convince me that using an observable event is not the right thing to do here?
Flags: needinfo?(mstange)
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Part 1: Add SuppressDisplayport painting and use it during tab switch. r=kats,mconley
Bug 1186662 - Suppress the Displayport during active resize on mac. r=mstange
Attachment #8647772 - Flags: review?(mstange)
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Part 1: Add SuppressDisplayport painting and use it during tab switch. r=kats,mconley
Comment on attachment 8647772 [details]
MozReview Request: Bug 1186662 - Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Suppress the Displayport during active resize on mac. r=mstange
Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange
Attachment #8647773 - Flags: review?(mstange)
Got it working for win32 resize but it's using using VM_ENTERSIZEMOVE message so I need to exclude the MOVE part. I expect I can use the first resize message to do this.
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Part 1: Add SuppressDisplayport painting and use it during tab switch. r=kats,mconley
Comment on attachment 8647773 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange
Bug 1186662 - Part 3: Implement Displayport suppresion for windows resize. r=kats
Attachment #8648965 - Flags: review?(bugmail.mozilla)
Comment on attachment 8648965 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

https://reviewboard.mozilla.org/r/16329/#review14629

This looks ok with the below comments addressed. However I'm not a window widget peer and am pretty unfamiliar with this code so I think somebody who knows this code should review as well.

::: widget/windows/nsWindow.h:561
(Diff revision 1)
> +  // Displayport Suppression

This comment looks out-of-place. I'd suggest:

"// Used for displayport suppression during window resize"

::: widget/windows/nsWindow.cpp:5315
(Diff revision 1)
> +          printf("Send\n");

Drop printf

::: widget/windows/nsWindow.cpp:5318
(Diff revision 1)
> +    }

Missing break statement

::: widget/windows/nsWindow.cpp:5335
(Diff revision 1)
> +          printf("end\n");

Drop printf

::: widget/windows/nsWindow.cpp:5339
(Diff revision 1)
> -        NotifySizeMoveDone();
> +          NotifySizeMoveDone();

If the code works the way I'm assuming, I think this call to NotifySizeMoveDone() should be outside the "if (mResizeState == RESIZING)" block, because you're ignoring the WM_MOVING messages. In other words, if the user moves the window but doesn't resize it, the old code would still send a NotifySizeMoveDone() but your new code does not.
Attachment #8648965 - Flags: review?(bugmail.mozilla)
Opps sorry kats, I forgot to clean-up & self-review the patch before publish it. Thanks for the feedback!

Removing ni?mstange since there's a review flag now.
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Part 1: Add SuppressDisplayport painting and use it during tab switch. r=kats,mconley
Comment on attachment 8647773 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange
Attachment #8648965 - Attachment description: MozReview Request: Bug 1186662 - Part 3: Implement Displayport suppresion for windows resize. r=kats → MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm
Attachment #8648965 - Flags: review?(jmathies)
Attachment #8648965 - Flags: review?(bugmail.mozilla)
Comment on attachment 8648965 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm
Comment on attachment 8648965 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

https://reviewboard.mozilla.org/r/16329/#review14641
Attachment #8648965 - Flags: review?(bugmail.mozilla) → review+
Attachment #8644581 - Attachment is obsolete: true
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

https://reviewboard.mozilla.org/r/15929/#review14817

r=me if you store the TabParent instead of the browser (since that doesn't account for swapped frame loaders). Nice green try run!
Attachment #8647227 - Flags: review?(mconley) → review+
Yes that fixed it. Thanks for the tip. (The oranges are from a bad m-c tip)
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Part 1: Add SuppressDisplayport painting and use it during tab switch. r=kats,mconley
Comment on attachment 8647773 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange
Comment on attachment 8648965 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm
Attachment #8647772 - Attachment is obsolete: true
Attachment #8647772 - Flags: review?(mstange)
Changed the title to match the patch term to make searching easier.
Summary: DisplayPort should shrink to the viewport during sync paint operation (Resize, Tab Switch, Scrollbar, PageLoad) → DisplayPort should be suppressed to the viewport during sync paint operation (Resize, Tab Switch, Scrollbar, PageLoad)
I'd like to see opt talos tresize results showing these changes don't regress the perf test.
Comment on attachment 8648965 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

https://reviewboard.mozilla.org/r/16329/#review14849

::: widget/windows/nsWindow.cpp:5307
(Diff revision 3)
> +    case WM_SIZING:

Do you rteally need this sizing handler? Windows is going to fire ton of these events when the user drags the window frame around, and you have this attached to a block of js. I think this is something we want to avoid or maybe, move the handler logic into c++.
Attachment #8648965 - Flags: review?(jmathies)
(In reply to Jim Mathies [:jimm] from comment #52)
> I'd like to see opt talos tresize results showing these changes don't
> regress the perf test.

I've already done measurements for tps (Tab switch) and they improve the score. TResize with e10s doesn't measure the content process size since the resizing is now async so I don't expect the test to really move since we're not measuring the async delay which this patch will improve.

(In reply to Jim Mathies [:jimm] from comment #53)
> Do you rteally need this sizing handler? Windows is going to fire ton of
> these events when the user drags the window frame around, and you have this
> attached to a block of js. I think this is something we want to avoid or
> maybe, move the handler logic into c++.

If you look at the simple state machine we will only fire one message when you start/end the window resize. I just need to make sure we get a resize event because it's ambiguous with the WM_ENTERSIZEMOVE message. We will explicitly not more than one event per resize otherwise we would get stuck in the suppressed state. I imagine it's fine to go through JS for something that happens once (well twice for the enter/exit message) per resize which is a 'rare' user interaction.
Flags: needinfo?(jmathies)
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/35f538038395489cafccbdfe18f0f86d682d8e9d
changeset:  35f538038395489cafccbdfe18f0f86d682d8e9d
user:       Benoit Girard <b56girard@gmail.com>
date:       Wed Aug 19 17:08:41 2015 -0400
description:
Bug 1186662 - Part 1: Add SuppressDisplayport painting and use it during tab switch. r=kats,mconley
Keywords: leave-open
(In reply to Benoit Girard (:BenWa) from comment #54)
> (In reply to Jim Mathies [:jimm] from comment #52)
> > I'd like to see opt talos tresize results showing these changes don't
> > regress the perf test.
> 
> I've already done measurements for tps (Tab switch) and they improve the
> score. TResize with e10s doesn't measure the content process size since the
> resizing is now async so I don't expect the test to really move since we're
> not measuring the async delay which this patch will improve.
> 
> (In reply to Jim Mathies [:jimm] from comment #53)
> > Do you rteally need this sizing handler? Windows is going to fire ton of
> > these events when the user drags the window frame around, and you have this
> > attached to a block of js. I think this is something we want to avoid or
> > maybe, move the handler logic into c++.
> 
> If you look at the simple state machine we will only fire one message when
> you start/end the window resize. I just need to make sure we get a resize
> event because it's ambiguous with the WM_ENTERSIZEMOVE message. We will
> explicitly not more than one event per resize otherwise we would get stuck
> in the suppressed state. I imagine it's fine to go through JS for something
> that happens once (well twice for the enter/exit message) per resize which
> is a 'rare' user interaction.

I see, so the if (mResizeState == IN_SIZEMOVE) check insures we don't fire more than one of these observer events per window frame drag? That sounds ok. I'll take a look at the rest here in a bit.
Flags: needinfo?(jmathies)
https://reviewboard.mozilla.org/r/16329/#review14911

::: widget/windows/nsWindow.cpp:5309
(Diff revision 3)
> +      if (mResizeState == IN_SIZEMOVE) {

nit - please add a comment here explaining what the purpose of this code is for future reference.

::: widget/windows/nsWindow.cpp:5311
(Diff revision 3)
> +        nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();

nit - check for 80 char limit here.

::: widget/windows/nsWindow.cpp:5332
(Diff revision 3)
> +        nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();

nit - ditto
Attachment #8648965 - Flags: review+
Comment on attachment 8647773 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

https://reviewboard.mozilla.org/r/16039/#review15281

Ship It!
Attachment #8647773 - Flags: review?(mstange) → review+
(In reply to Benoit Girard (:BenWa) from comment #25)
> Can you convince me that using an observable event is not the right thing to
> do here?

Using an observer instead of an event is much simpler, so I'm approve of your solution here. The one drawback it has is that resizing any window will suppress the display port of all tabs in all windows, not just in the resized window. However, since suppressing the display port doesn't trigger a paint on its own, and since you can't interact with background windows while resizing the foreground one, that's probably ok.
Flags: needinfo?(mstange)
The leak was caused by bug 1198831.
Comment on attachment 8647227 [details]
MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange
Attachment #8647227 - Attachment description: MozReview Request: Bug 1186662 - Part 1: Add SuppressDisplayport painting and use it during tab switch. r=kats,mconley → MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange
Comment on attachment 8647773 [details]
MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm
Attachment #8647773 - Attachment description: MozReview Request: Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange → MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm
Attachment #8648965 - Attachment is obsolete: true
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/d12a7b9478a467430364c058285896f7546a0f7e
changeset:  d12a7b9478a467430364c058285896f7546a0f7e
user:       Benoit Girard <b56girard@gmail.com>
date:       Wed Aug 26 12:59:17 2015 -0400
description:
Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d888155e18c7b56d4b47c17a499a5f88308db5
changeset:  f8d888155e18c7b56d4b47c17a499a5f88308db5
user:       Benoit Girard <b56girard@gmail.com>
date:       Mon Aug 17 14:24:59 2015 -0700
description:
Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm
Depends on: 1202503
We're going to do async scrollbar so we wont be doing it for that. I'm not convinced doing for pageload is worth while, I don't know actually, so that should probably get it's own bug.
Keywords: leave-open
Summary: DisplayPort should be suppressed to the viewport during sync paint operation (Resize, Tab Switch, Scrollbar, PageLoad) → DisplayPort should be suppressed to the viewport during sync paint operation (Resize, Tab Switch)
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: 1205025
You need to log in before you can comment on or make changes to this bug.