990916 - Remove displayport from elements that are not actively scrolled

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

For things that become actively scrolled, the APZC sets a displayport on the item. However the displayport is never removed so even if the item is no longer actively scrolled it is probably painting that area and wasting resources unnecessarily.

Comment 1

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

We should probably fix this sooner rather than later.

Comment 2

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

Attachment: Patch

This is a small patch that doesn't try to do all the things from bug 1009786. It seems to work ok in my basic testing (using http://people.mozilla.org/~kgupta/gridframe.html and http://people.mozilla.org/~kgupta/gridframe2.html).

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4e8428370e7 - I expect at some of the reftests to break but maybe the 4s timeout is long enough that it won't impact the tests?

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8711134 [details] [diff] [review]
Patch

We set displayport base on any scrollframe regardless of presence of displayport, so probably don't clear the base.

Usually we mark a scroll frame as "not scrolled recently" after 3-4 seconds. This seems too short for removing a displayport. I think it's normal for a user to pause for more than 3 seconds before continuing to scroll. Throwing out displayport contents, only having to repaint them a little later seems like a waste.

The interaction with textoverflow needs to be looked at. During display list building TextOverflow objects may set the NS_SCROLLFRAME_INVALIDATE_CONTENTS_ON_SCROLL bit on scrollframes. This will make us call MarkNotRecentlyScrolled here

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=0947272393af#2926

also during display list building. I really don't want to add any more changes to existence of displayports that happen during painting. It's already a confusing nightmare.

Comment 4

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

Thanks. Sounds like I can address both of those issues by moving my new code into a separate function on a separate timer. The timer would kicked off at the same time, but wouldn't be tied to MarkNotRecentlyScrolled, so it wouldn't have the weird interaction with TextOverflow stuff. We can adjust the timeout for the timer separately, and cancel it if the user starts scrolling again.

Comment 5

[Timothy Nikkel (:tnikkel)]

Sounds good.

Comment 6

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

Attachment: Patch v2

Updated as per discussion. I also made the timeout a pref. I still need to write a test for this, I think I should be able to do it using APZTestData stuff we have.

Comment 7

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

I made some modifications to test_layerization.html to test this de-layerization and it works but it needs setTimeout calls. I also realized there might be issues with propagating the de-layerization up the ancestor chain of scrollables, and blocking it if there is a layerized descendant. I'll think about this a bit more.

Comment 8

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

Updated patchset, to what I think should be correct. I also updated test_layerization to test the new code. Got it working locally but pushed to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e64822b2222c to hammer it a bit more before requesting review.

Comment 9

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

That try push showed a consistent failure in a different test which to me looks like it shouldn't be caused by my patches. Digging..

Comment 10

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

Ok, I think I fixed the problem. Latest try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=99bc8576ed98&group_state=expanded

Comment 11

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

Attachment: Part 1 - Fix latent issue in test_bug976963.html

Small fix for test_bug976963.html - a patch later in this series changes timing of the tests sufficiently that test_bug976963.html ends up getting a synthetic mousemove event right at the end of the test, and it doesn't expect that and fails. So I just disable synthetic mousemoves for this test, which I think is reasonable.

Comment 12

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

Attachment: Part 2 - Track if a scrollframe is a parent

Note that the call to ForceLayerToScrollParent() now happens even with root frame containers, but the old use site of that (the call to ShouldForceLayerForScrollParent()) is still guarded by the containerless check, so it should have no effect on the existing flows.

Comment 13

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

Attachment: Part 3 - Add a displayport expiry timer

Comment 14

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

Attachment: Part 4 - On expiry, trigger expiry up the chain

The part 3 patch only causes displayport expiration on "leaf" scrollframes (those that are not mIsScrollParent). This patch triggers displayport expiry up the ancestor chain. If the timeout is 15 seconds, then the leaf scrollframe will expire 15s after the scrolling, and its ancestor would expire 30s after the scrolling, assuming that ancestor had no _other_ descendants that were still active.

Comment 15

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

Attachment: Part 5 - Update test_layerization to test for displayport expiry

There's some bits here that I'm not really happy with but I couldn't think of anything better. Open to suggestions. It seems to work reliably enough, at least.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8713161 [details] [diff] [review]
Part 3 - Add a displayport expiry timer

Review of attachment 8713161 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2390,5 @@
> +    if (!mDisplayPortExpiryTimer) {
> +      return;
> +    }
> +  }
> +  mDisplayPortExpiryTimer->InitWithFuncCallback(

If the timer is already set, does this reset the timer to fire at the new timeout? Or will the timer fire twice, both at the old and at the new timeout?

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8713162 [details] [diff] [review]
Part 4 - On expiry, trigger expiry up the chain

Review of attachment 8713162 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +3194,5 @@
> +    MOZ_ASSERT(scrollAncestor->WantAsyncScroll() ||
> +      frame->PresContext()->PresShell()->GetRootScrollFrame() == frame);
> +    if (nsLayoutUtils::AsyncPanZoomEnabled(frame) &&
> +        nsLayoutUtils::HasDisplayPort(frame->GetContent())) {
> +      scrollAncestor->TriggerDisplayPortExpiration();

What if scrollAncestor has another descendant with a display port that has not expired yet?

Comment 18

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

Comment on attachment 8713161 [details] [diff] [review]
Part 3 - Add a displayport expiry timer

Review of attachment 8713161 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2390,5 @@
> +    if (!mDisplayPortExpiryTimer) {
> +      return;
> +    }
> +  }
> +  mDisplayPortExpiryTimer->InitWithFuncCallback(

It should cancel the existing one and schedule a new timeout. https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/xpcom/threads/nsTimerImpl.cpp#281

Comment 19

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

Comment on attachment 8713162 [details] [diff] [review]
Part 4 - On expiry, trigger expiry up the chain

Review of attachment 8713162 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +3194,5 @@
> +    MOZ_ASSERT(scrollAncestor->WantAsyncScroll() ||
> +      frame->PresContext()->PresShell()->GetRootScrollFrame() == frame);
> +    if (nsLayoutUtils::AsyncPanZoomEnabled(frame) &&
> +        nsLayoutUtils::HasDisplayPort(frame->GetContent())) {
> +      scrollAncestor->TriggerDisplayPortExpiration();

The TriggerDisplayPortExpiration call will schedule the timer, and when the timer kicks in the RemoveDisplayPortCallback again checks to see if mIsScrollParent is true, and aborts if it is. That means if the scrollAncestor is still a scroll parent at that time, it's displayport shouldn't get removed.

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8713162 [details] [diff] [review]
Part 4 - On expiry, trigger expiry up the chain

Review of attachment 8713162 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +3194,5 @@
> +    MOZ_ASSERT(scrollAncestor->WantAsyncScroll() ||
> +      frame->PresContext()->PresShell()->GetRootScrollFrame() == frame);
> +    if (nsLayoutUtils::AsyncPanZoomEnabled(frame) &&
> +        nsLayoutUtils::HasDisplayPort(frame->GetContent())) {
> +      scrollAncestor->TriggerDisplayPortExpiration();

Ah, I missed the mIsScrollParent check in RemoveDisplayPortCallback.

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8713163 [details] [diff] [review]
Part 5 - Update test_layerization to test for displayport expiry

Review of attachment 8713163 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/test/mochitest/test_layerization.html
@@ +71,5 @@
>  
>  var gTestContinuation = null;
>  var utils;
>  
> +const DISPLAYPORT_EXPIRY = 1000;

Can we reduce this value to avoid the test taking long to run? If we address the "not painting fast enough" issue as I describe below, I don't think reducing this value should be a problem.

@@ +111,5 @@
> +    // If we get here, it means that the round-trip time for a paint to go to
> +    // APZ and back was longer than DISPLAYPORT_EXPIRY ms, and so the
> +    // RequestContentRepaint in APZ triggered by the scrollTop++ might cause
> +    // displayport margins to get set again, after the expiry has removed them.
> +    // In this case, try doing this again, up to 3 times.

As suggested on IRC, it might be a good idea to have the setting of a displayport start the timer if it's not already started, and have scroll events re-set the timer. This way, if a repaint happens after the timer from the most recent scroll event has expired (which can happen not only if the paint from the scroll took very long, but also if something (like an input event) triggered a repaint but not a scroll), the displayport is still cleared.

Note that this isn't a comment about the behaviour of the test per say, but rather a comment about the production behaviour being tested; once we agree on the desirable production behaviour, I'm happy with doing whatever is needed in the test.

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8713163 [details] [diff] [review]
Part 5 - Update test_layerization to test for displayport expiry

Review of attachment 8713163 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the issues mentioned in the previous comment, the test looks pretty good - thanks!

Comment 23

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

Comment on attachment 8713161 [details] [diff] [review]
Part 3 - Add a displayport expiry timer

This patch is going to change a bit.

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8713160 [details] [diff] [review]
Part 2 - Track if a scrollframe is a parent

Should probably init mIsScrollParent in the constructor.

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> Comment on attachment 8713162 [details] [diff] [review]
> Part 4 - On expiry, trigger expiry up the chain
> 
> Review of attachment 8713162 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsLayoutUtils.cpp
> @@ +3194,5 @@
> > +    MOZ_ASSERT(scrollAncestor->WantAsyncScroll() ||
> > +      frame->PresContext()->PresShell()->GetRootScrollFrame() == frame);
> > +    if (nsLayoutUtils::AsyncPanZoomEnabled(frame) &&
> > +        nsLayoutUtils::HasDisplayPort(frame->GetContent())) {
> > +      scrollAncestor->TriggerDisplayPortExpiration();
> 
> The TriggerDisplayPortExpiration call will schedule the timer, and when the
> timer kicks in the RemoveDisplayPortCallback again checks to see if
> mIsScrollParent is true, and aborts if it is. That means if the
> scrollAncestor is still a scroll parent at that time, it's displayport
> shouldn't get removed.

But for that sequence to work we have to have had another paint so that mIsScrollParent gets updated. Since this is happening after 15 seconds of inactivity seems like no painting would be common.

Comment 26

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

(In reply to Botond Ballo [:botond] from comment #21)
> As suggested on IRC, it might be a good idea to have the setting of a
> displayport start the timer if it's not already started, and have scroll
> events re-set the timer.

So this made sense from a production code point of view, and I implemented it. However it didn't really with respect to the sync-scrolling portion of the test because sync scrolling might not result in the margins being updated, and so now it doesn't necessarily kick off the timer. I ended up just using async scrolling to trigger the expiry.

Try push to verify: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8fcd0b2ac06

Comment 27

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

(In reply to Timothy Nikkel (:tnikkel) from comment #25)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> But for that sequence to work we have to have had another paint so that
> mIsScrollParent gets updated. Since this is happening after 15 seconds of
> inactivity seems like no painting would be common.

So whenever we remove a displayport, we schedule a new paint. That paint would set mIsScrollParent in the ancestor to false. And then 15s later the timer would expire, and we would see that mIsScrollParent is false in the ancestor, and we would remove the ancestor's displayport.

Comment 28

[Timothy Nikkel (:tnikkel)]

Ah, okay, thanks for the explanation.

Comment 29

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

Needed a small tweak to the test so that outer1 could get scrolled twice without also causing inner1 to scroll. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28a7baaba856

Comment 30

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

Attachment: Part 3 - When setting a displayport, schedule an expiry timer (v2)

Comment 31

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

Attachment: Part 4 - On expiry, trigger expiry up the chain (v2)

Comment 32

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

Attachment: Part 5 - Update test_layerization to test for displayport expiry (v2)

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 8715502 [details] [diff] [review]
Part 5 - Update test_layerization to test for displayport expiry (v2)

Review of attachment 8715502 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/test/mochitest/test_layerization.html
@@ +182,5 @@
> +  yield scrollWheelOver(document.getElementById('outer3').contentDocument.getElementById('inner3'));
> +  yield waitForAllPaints(function() {
> +    flushApzRepaints(driveTest);
> +  });
> +  ok(isLayerized('inner3'), "inner3 remains unlayerized after sync scroll and expiry");

description needs updating ("remains unlayerized", "sync scroll")

@@ +185,5 @@
> +  });
> +  ok(isLayerized('inner3'), "inner3 remains unlayerized after sync scroll and expiry");
> +  yield setTimeout(driveTest, DISPLAYPORT_EXPIRY);
> +  yield waitForAllPaints(callDriveTestAsync);
> +  ok(!isLayerized('inner3'), "inner3 remains unlayerized after sync scroll and expiry");

ditto

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8715502 [details] [diff] [review]
Part 5 - Update test_layerization to test for displayport expiry (v2)

Bugzilla's been doing weird things when two r+'s clash. Anyways, restoring tn's r+.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb811e5bf97
https://hg.mozilla.org/integration/mozilla-inbound/rev/1be96090275a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8df453ece1d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4e4bbafd3d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a13a3af27e80

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3eb811e5bf97
https://hg.mozilla.org/mozilla-central/rev/1be96090275a
https://hg.mozilla.org/mozilla-central/rev/8df453ece1d4
https://hg.mozilla.org/mozilla-central/rev/c4e4bbafd3d3
https://hg.mozilla.org/mozilla-central/rev/a13a3af27e80

Comment 37

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

I don't think this is worth uplifting to 46 - although it may provide some memory improvements it's also a little risky and could benefit from the extra bake time.