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

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 2

[Benoit Girard (:BenWa)]

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.

Comment 3

[Benoit Girard (:BenWa)]

I added pageload in here. It's likely ATM the only sync operation that Talos would measure properly using the tp5o pageset.

Comment 4

[Benoit Girard (:BenWa)]

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?

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Experiment

Here's my failed attempt so far.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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).

Comment 7

[Benoit Girard (:BenWa)]

It's not the triggering that I'm having problems with. It's how to set/get that we're in a 'sync' operation.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 9

[Benoit Girard (:BenWa)]

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.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: tps with default display port

Comment 11

[Benoit Girard (:BenWa)]

Attachment: tps with no display port

Comment 12

[Benoit Girard (:BenWa)]

The scores show a clear improvement with no display port. I ran by patching 'GetDisplayPort' to false. Are you convinced kats by this data?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Yes, thank you!

Comment 14

[Benoit Girard (:BenWa)]

Attachment: tps with v1

Comment 15

[Benoit Girard (:BenWa)]

Attachment: patch v1 (WIP)

Comment 16

[Benoit Girard (:BenWa)]

Attachment: 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 17

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 18

[Benoit Girard (:BenWa)]

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 19

[Benoit Girard (:BenWa)]

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 20

[Benoit Girard (:BenWa)]

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 21

[Kartikaya Gupta (email:[e-mail])]

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)

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Oh also in the commit message s/Prioritize/Suppress/

Comment 23

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 24

[Benoit Girard (:BenWa)]

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 25

[Benoit Girard (:BenWa)]

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?

Comment 26

[Benoit Girard (:BenWa)]

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 27

[Benoit Girard (:BenWa)]

Attachment: 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

Comment 28

[Benoit Girard (:BenWa)]

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 29

[Benoit Girard (:BenWa)]

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

Comment 30

[Benoit Girard (:BenWa)]

Attachment: 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 31

[Benoit Girard (:BenWa)]

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 32

[Benoit Girard (:BenWa)]

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 33

[Benoit Girard (:BenWa)]

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 34

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1186662 - Part 3: Suppress the Displayport during active resize on Windows. r=kats,jimm

Bug 1186662 - Part 3: Implement Displayport suppresion for windows resize. r=kats

Comment 35

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9d985900fb2

Comment 36

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 37

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e35c4442dee9

Comment 38

[Benoit Girard (:BenWa)]

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 39

[Benoit Girard (:BenWa)]

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 40

[Benoit Girard (:BenWa)]

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 41

[Benoit Girard (:BenWa)]

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 42

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0328036269b9

Comment 43

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d1521eb7abc

Comment 44

[Kartikaya Gupta (email:[e-mail])]

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

Comment 45

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45c9ca8a5892

Comment 46

[Mike Conley (:mconley) (:⚙️)]

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!

Comment 47

[Benoit Girard (:BenWa)]

Yes that fixed it. Thanks for the tip. (The oranges are from a bad m-c tip)

Comment 48

[Benoit Girard (:BenWa)]

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 49

[Benoit Girard (:BenWa)]

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 50

[Benoit Girard (:BenWa)]

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 51

[Benoit Girard (:BenWa)]

Changed the title to match the patch term to make searching easier.

Comment 52

[Jim Mathies [:jimm]]

I'd like to see opt talos tresize results showing these changes don't regress the perf test.

Comment 53

[Jim Mathies [: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/#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++.

Comment 54

[Benoit Girard (:BenWa)]

(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.

Comment 55

[Benoit Girard (:BenWa)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/35f538038395489cafccbdfe18f0f86d682d8e9d
changeset:  35f538038395489cafccbdfe18f0f86d682d8e9d
user:       Benoit Girard <[e-mail]>
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

Comment 56

[Jim Mathies [:jimm]]

(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.

Comment 57

[Jim Mathies [:jimm]]

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

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35f538038395

Comment 59

[Markus Stange [:mstange]]

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!

Comment 60

[Markus Stange [:mstange]]

(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.

Comment 61

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68a36f7ab1f4

Comment 62

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9203f8b8924

Comment 63

[Benoit Girard (:BenWa)]

The leak was caused by bug 1198831.

Comment 64

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a9ca05c3366

Comment 65

[Benoit Girard (:BenWa)]

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

Comment 66

[Benoit Girard (:BenWa)]

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

Comment 67

[Benoit Girard (:BenWa)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/d12a7b9478a467430364c058285896f7546a0f7e
changeset:  d12a7b9478a467430364c058285896f7546a0f7e
user:       Benoit Girard <[e-mail]>
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 <[e-mail]>
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

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d12a7b9478a4
https://hg.mozilla.org/mozilla-central/rev/f8d888155e18

Comment 69

[Benoit Girard (:BenWa)]

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.