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

STR:
1) Turn on the minimap.
2) Scroll on some pages long pages near the top/bottom with the scrollbar.

you will notice that, at least according to the minimap, we're shrinking the display port incorrectly and temporarily. If this is correct we're throwing out valid content and repainting it later.

I see this on gmail.com in my inbox list. The display port is large enough to fit the entire page without repainting but this bug might be causing us to paint as we scroll anyways.

I don't get this using the trackpad scrolling.

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

It's pretty hard to tell from your gif but most likely what you're seeing here is that when the user scrolls fast we shrink the displayport margins to paint faster and keep up better. It will perform worse in cases where the user scrolls super-fast and then abruptly reverses direction or stops scrolling but that should be a less common scenario than scrolling super-fast and letting it drift to a slower speed.

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

I'm not sure that's it but it's hard to tell. I don't think it's even implemented yet on mac. It also only occurs with scrollbar and not flings.

Also there's really trivial scenarios that it should be handling without hitting this bug. If the page is just the right size the entire page fits within the display port. I still see us shrink the displayport when we have the entire page painted.

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

Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

https://reviewboard.mozilla.org/r/17345/#review15503

::: layout/base/nsLayoutUtils.cpp:923
(Diff revision 1)
> +  nsRect frameRect = nsLayoutUtils::CalculateExpandedScrollableRect(frame);

Please call this expandedScrollableRect or scrollableRect rather than frameRect. The frame rect implies something different.

::: layout/base/nsLayoutUtils.cpp:926
(Diff revision 1)
> +  ScreenPoint screenScrollPos = LayoutDevicePoint::FromAppUnits(scrollPos,

To reduce rounding problems I would prefer if you subtracted the scrollPos in app units and then did a single conversion to Screen pixels.

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

https://reviewboard.mozilla.org/r/17345/#review15601

Thanks!

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

https://reviewboard.mozilla.org/r/17345/#review15601

Thanks!

	Pulsebot
	Comment 9 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/758f3df6c016

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

https://hg.mozilla.org/mozilla-central/rev/758f3df6c016

	William Lachance (:wlach)
	Comment 11 • 9 years ago

This caused a regression in the tcheck numbers: bug 1199683.

	Pulsebot
	Comment 12 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f25ab0817bab

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 13 • 9 years ago

we lost some nice e10s improvements to windows tp5 scroll and tscroll-asap, but fixed an android tcheck2 regression:
http://alertmanager.allizom.org:8080/alerts.html?rev=f25ab0817babf1dcbf71d9e46f80c85ca9b55cdb&showAll=1&testIndex=0&platIndex=0 (the regressions here are items that were improvements a few days ago)

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

Still trying to fix this. It's possible the proper patch to do this wont have tcheck2 regressions:

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

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

I’m not sure how to re-request review with MozReview. I no longer ForceInside if we’re not tiling because it doesn’t work well and I fixed the emulator oranges + perma orange issue.

I think in the future we're going to want to move the displayport in increments (even if we don't tile) so I'm not overly interested in fixing that.

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

This patch is wrong. I need to think about this some more.

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

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

Haven't looked at the new patch yet but https://bugzilla.mozilla.org/show_bug.cgi?id=1194851#c15 might explain why you've been having so much trouble with this...

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

https://reviewboard.mozilla.org/r/17345/#review16907

::: layout/base/nsLayoutUtils.cpp:925
(Diff revision 4)
> +  ScreenRect screenExpScrollableRect =

Move this into the if (tiles) block since it's not used anywhere outside of it

::: layout/base/nsLayoutUtils.cpp:929
(Diff revision 4)
> +  ScreenPoint scrollPosScreen = LayoutDevicePoint::FromAppUnits(scrollPos, auPerDevPixel)

Move this back inside the if (tiles) block where it was before; it's not used outside anywhere

::: layout/base/nsLayoutUtils.cpp:967
(Diff revision 4)
> -    float w = alignmentX * ceil(screenRect.XMost() / alignmentX) - x;
> +    float w = alignmentX * ceil(screenRect.width / alignmentX + 1);

Can you provide examples of numbers where this produces a more desirable result? As far as I can tell this will give a width one tile wider than before in most cases and I don't see why that is desirable.

e.g. for alignmentX = 10, screenRect.x = 12, screenRect.width = 83. This used to give x = 10, w = 90 and with your change it gives x = 10, w = 100.

And regardless it seems this change is orthogonal to everything else in this patch and should be moved to a separate bug if there's a good reason for it.

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

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> ::: layout/base/nsLayoutUtils.cpp:967
> (Diff revision 4)
> > -    float w = alignmentX * ceil(screenRect.XMost() / alignmentX) - x;
> > +    float w = alignmentX * ceil(screenRect.width / alignmentX + 1);
> 
> Can you provide examples of numbers where this produces a more desirable
> result? As far as I can tell this will give a width one tile wider than
> before in most cases and I don't see why that is desirable.

The problem is we're aligning the two edges independently from each other. In practice (as show by the minimap) this means that when you scroll, we will shrink the display port on one frame, then increase it after scrolling a few pixels. If the unaligned displayport isn't changing size, we shouldn't change the size of the aligned display port. This is bad for recycling and performance.

> And regardless it seems this change is orthogonal to everything else in this
> patch and should be moved to a separate bug if there's a good reason for it.

It's also causing the display port to change size sporadically causing it to disagree with the compositor and shrink. It's required to fix this bug.

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

(In reply to Benoit Girard (:BenWa) from comment #21)
> The problem is we're aligning the two edges independently from each other.
> In practice (as show by the minimap) this means that when you scroll, we
> will shrink the display port on one frame, then increase it after scrolling
> a few pixels.

Can you provide specific numbers that illustrate this problem?

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

patch (to make the logging clearer):

     screenRect += scrollPosScreen;
+    printf("Before %s\n", ToString(screenRect).c_str());
     float x = alignmentX * floor(screenRect.x / alignmentX);
     float y = alignmentY * floor(screenRect.y / alignmentY);
     float w = alignmentX * ceil(screenRect.XMost() / alignmentX) - x;
     float h = alignmentY * ceil(screenRect.YMost() / alignmentY) - y;
     screenRect = ScreenRect(x, y, w, h);
+    printf("After %s\n", ToString(screenRect).c_str());
     screenRect -= scrollPosScreen;

Before (-1,3252.78,1887,6977.5)
After (-512,3072,2560,7168)

Before (-1,3283.48,1887,6977.5)
After (-512,3072,2560,7680)

Problem is pretty clear from watching the minimap and it matches up with the invalidation flashes.

If we prefer not rounding out but snapping to the closest point I can do that implementation instead if that's your concern.

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

Thanks! I understand the problem better now, and I agree that your changes there are fine. Rounding out should be ok. r+ with the other nits from comment 20 addressed.

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

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

Comment on attachment 8653034 [details]
MozReview Request: Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

I addressed the comment 20 nits. This should be r=kats.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a259b679e13556208f53378ad0b1ad312c68c672
Bug 1191539 - DisplayPort should ForceInside frameRect to match compositor DisplayPort. r=kats

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 28 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/a259b679e135

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

The patch is triggered major (10%-20%) TP5/Page switch regressions. Those test would be impacted by a larger displayport while not gaining any benefit. This is probably because we're rounding-out the display port. We will have to do an adjustment. I didn't think the results would be so severe.

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

Alright I checked locally. I don't think the problem is the rounding anymore.

I think the issue is that we ForceInside which moves the displayport down during the page load. Effectively this patches causes us to paint a displayport on page load where before we effectively had a suppressed displayport.

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

If we don't mind I'd like for us to accept this regression. Before we were getting the right thing we want by accident.

I filed bug 1205025 to intentionally suppress the displayport.

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

(Meta-comment: I feel like even when we are able to reproduce these regressions locally it's pretty hard to be certain that we can diagnose them properly... do we need more tooling to be able to track down these perf regressions with certainty?)

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

Looking bad, the + 1 wasn't necessary in the end. It's preventing the displayport suppression from properly working, adding an extra row of tiles below.

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

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/61baabe617c9786b3ce315709a39fa17e78b15fc
Bug 1191539 - Don't further increase the size of the displayport when rounding up. r=kats

	Carsten Book [:Tomcat]
	Comment 36 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/61baabe617c9

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

and the regressions are gone!

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

Comment on attachment 8661445 [details] [diff] [review]
patch

This doesn't work the way you intended it to. Try it with an alignment of 10, 10 and a rect of (9, 9, 10, 10).

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

Backed out the last part: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e112d30103e

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 40 • 9 years ago

are we going to accept these tps and tp5 main rss regressions on osx:
http://alertmanager.allizom.org:8080/alerts.html?rev=6e112d30103e1f13d93f3380d83c50485b30b172&showAll=1&testIndex=0&platIndex=0

I am fine either way, it looks like we tried and then backed things out.  Do let me know what the plan is.

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

I think we should accept the regressions at least until Benoit comes back from vacation and has a chance to take another look.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 42 • 9 years ago

sounds good, we will let BenWa make the call here!

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

Merge of the backout of the last part: https://hg.mozilla.org/mozilla-central/rev/6e112d30103e

The bug as filed is still fixed. We'll need to fix the perf regressions in a separate bug.

	Timothy Nikkel (:tnikkel)
	Comment 44 • 9 years ago

Bug 957668 used Intersect when it should have used ForceInside, which is what caused this bug, marking dependency.