Closed Bug 990916 Opened 6 years ago Closed 4 years ago

Remove displayport from elements that are not actively scrolled

Categories

(Core :: Layout, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 --- unaffected
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 5 obsolete files)

1.22 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.28 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
15.79 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.55 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.20 KB, patch
botond
: review+
botond
: review+
Details | Diff | Splinter Review
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.
Blocks: 1009786
No longer blocks: 1009786
Depends on: 1009786
Component: Panning and Zooming → Layout
We should probably fix this sooner rather than later.
Assignee: nobody → bugmail.mozilla
Blocks: apz-desktop
Attached patch Patch (obsolete) — Splinter Review
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?
Attachment #8711134 - Flags: review?(tnikkel)
Attachment #8711134 - Flags: review?(mstange)
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.
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.
Sounds good.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8711134 - Attachment is obsolete: true
Attachment #8711134 - Flags: review?(tnikkel)
Attachment #8711134 - Flags: review?(mstange)
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.
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.
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..
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.
Attachment #8713157 - Flags: review?(bugs)
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.
Attachment #8713160 - Flags: review?(tnikkel)
Attachment #8711195 - Attachment is obsolete: true
Attachment #8713161 - Flags: review?(tnikkel)
Attachment #8713161 - Flags: review?(mstange)
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.
Attachment #8713162 - Flags: review?(tnikkel)
Attachment #8713162 - Flags: review?(mstange)
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.
Attachment #8713163 - Flags: review?(tnikkel)
Attachment #8713163 - Flags: review?(botond)
Attachment #8713161 - Flags: review?(mstange) → review+
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 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 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 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.
Attachment #8713157 - Flags: review?(bugs) → review+
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.
Attachment #8713162 - Flags: review?(mstange) → review+
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 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!
Attachment #8713163 - Flags: review?(botond) → feedback+
Comment on attachment 8713161 [details] [diff] [review]
Part 3 - Add a displayport expiry timer

This patch is going to change a bit.
Attachment #8713161 - Flags: review?(tnikkel)
Attachment #8713162 - Flags: review?(tnikkel)
Attachment #8713163 - Flags: review?(tnikkel)
Comment on attachment 8713160 [details] [diff] [review]
Part 2 - Track if a scrollframe is a parent

Should probably init mIsScrollParent in the constructor.
Attachment #8713160 - Flags: review?(tnikkel) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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
(In reply to Timothy Nikkel (:tnikkel) from comment #25)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Ah, okay, thanks for the explanation.
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
Attachment #8713162 - Attachment is obsolete: true
Attachment #8715501 - Flags: review?(tnikkel)
Attachment #8713163 - Attachment is obsolete: true
Attachment #8715502 - Flags: review?(tnikkel)
Attachment #8715502 - Flags: review?(botond)
Attachment #8715500 - Flags: review?(tnikkel) → review+
Attachment #8715501 - Flags: review?(tnikkel) → review+
Attachment #8715502 - Flags: review?(tnikkel) → review+
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
Attachment #8715502 - Flags: review?(tnikkel)
Attachment #8715502 - Flags: review?(botond)
Attachment #8715502 - Flags: review+
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+.
Attachment #8715502 - Flags: review?(tnikkel) → review+
Depends on: 1245830
Depends on: 1246997
Blocks: 1009786
No longer depends on: 1009786
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.
Depends on: 1250924
Depends on: 1263416
You need to log in before you can comment on or make changes to this bug.