Benoit Girard (:BenWa) Assignee
	Description • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 1 • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 2 • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 3 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 4 • 9 years ago

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?

	Benoit Girard (:BenWa) Assignee
	Comment 5 • 9 years ago

Here's my failed attempt so far.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 7 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 8 • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 9 • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 12 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 13 • 9 years ago

Yes, thank you!

	Benoit Girard (:BenWa) Assignee
	Comment 16 • 9 years ago

Bug 1186662 - Part 1: Prioritize DisplayPort painting during tab switch. r?kats

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 17 • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 18 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 19 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 20 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 21 • 9 years ago

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)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 22 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 23 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 24 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 25 • 9 years ago

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?

	Benoit Girard (:BenWa) Assignee
	Comment 26 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 27 • 9 years ago

Bug 1186662 - Suppress the Displayport during active resize on mac. r=mstange

	Benoit Girard (:BenWa) Assignee
	Comment 28 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 29 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 30 • 9 years ago

Bug 1186662 - Part 2: Suppress the Displayport during active resize on mac. r=mstange

	Benoit Girard (:BenWa) Assignee
	Comment 31 • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 32 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 33 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 34 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 35 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 36 • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 37 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 38 • 9 years ago

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.

	Benoit Girard (:BenWa) Assignee
	Comment 39 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 40 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 41 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 42 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 43 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 44 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 45 • 9 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 46 • 9 years ago

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!

	Benoit Girard (:BenWa) Assignee
	Comment 47 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 48 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 49 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 50 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 51 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 52 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 53 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 54 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 55 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 56 • 9 years ago

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

	Jim Mathies [:jimm]
	Comment 57 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 58 • 9 years ago

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

	Markus Stange [:mstange]
	Comment 59 • 9 years ago

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!

	Markus Stange [:mstange]
	Comment 60 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 61 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 62 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 63 • 9 years ago

The leak was caused by bug 1198831.

	Benoit Girard (:BenWa) Assignee
	Comment 64 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 65 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 66 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 67 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 68 • 9 years ago

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

	Benoit Girard (:BenWa) Assignee
	Comment 69 • 9 years ago

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.