1257641 - Move the scroll offset update message to an empty transaction

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Bug 1253860 added a scroll offset update message that main-thread could send to APZ to "scroll" without the overhead of a full main-thread display list build and paint. The code for that was kind of hacked in, and had to be disabled for a few scenarios (bug 1256727 and bug 1255856) because it happened out-of-order with respect to other things in the main-thread pipeline.

Markus suggested that we move this message into an empty transaction that happens at the same point in the main-thread tick cycle as a regular layers transaction, which seems cleaner and less prone to issues. This bug is to track that work.

Comment 1

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

Taking so I don't forget about this, but if somebody else wants to implement it in the next few days feel free to steal.

Comment 2

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

After digging around the code a bit with Botond and Markus it seems like we need to do the following:
- Instead of sending this extra message, set a "pending scroll offset" on the layer, and schedule a PAINT_COMPOSITE_ONLY paint
- During ApplyPendingUpdatesForThisTransaction, update the framemetrics with the new scroll offset that was saved as the "pending scroll offset"

Comment 3

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

I wrote it up and it seems to work ok in my rudimentary local testing. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc997f4f5629

Comment 4

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

Actually that's not quite right. It seems like the async transforms are coming out empty from these empty-transaction updates, because mLastContentPaintMetrics is getting updated to the new scroll offset when it shouldn't really be, or something. Will look a bit more.

Comment 5

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

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

I have a test thrown in there but it fails intermittently because the displayport suppression for new pageloads deactivates and triggers a paint at $random_time. I need to find a way to disable that for this test.

Comment 6

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

Attachment: MozReview Request: Bug 1257641 - Replace the mUpdateScrollOffset bool with an enum, needed in the next patch. r?botond

Review commit: https://reviewboard.mozilla.org/r/45513/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45513/

Comment 7

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

Attachment: MozReview Request: Bug 1257641 - Use empty transactions to carry scroll offset updates to APZ that don't require a repaint. r?mattwoodrow,mstange,botond

Review commit: https://reviewboard.mozilla.org/r/45515/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45515/

Comment 8

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

Attachment: MozReview Request: Bug 1257641 - Remove now-unused code for the lightweight scroll offset update message. r?botond

Review commit: https://reviewboard.mozilla.org/r/45517/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45517/

Comment 9

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

Attachment: MozReview Request: Bug 1257641 - Allow enabling/disabling of displayport suppression for tests. r?botond

Review commit: https://reviewboard.mozilla.org/r/45519/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45519/

Comment 10

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

Attachment: MozReview Request: Bug 1257641 - Add a test for APZ paint-skipping and event transformations after a skipped paint. r?botond

Review commit: https://reviewboard.mozilla.org/r/45521/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45521/

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8740031 [details]
MozReview Request: Bug 1257641 - Use empty transactions to carry scroll offset updates to APZ that don't require a repaint. r?mattwoodrow,mstange,botond

https://reviewboard.mozilla.org/r/45515/#review42053

Comment 12

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/45521/#review42055

::: gfx/layers/apz/test/mochitest/helper_scrollto_tap.html:31
(Diff revision 1)
> +  var elem = document.documentElement;
> +  var skipping = location.search == '?true';
> +  utils.checkAndClearPaintedState(elem);
> +  window.scrollTo(0, 20);
> +  waitForAllPaints(function() {
> +    window.opener.is(utils.checkAndClearPaintedState(elem), !skipping, "Document element didn't get painted; paint-skipping worked");

I don't think this is conclusive. It's good to check that we didn't invalidate anything, but I think this check would pass even without paint skipping, assuming we haven't moved the display port. This check checks whether we actually ended up invalidating + painting something, but paint skipping is about not doing display list building + layer building.

I think all I'm asking for is for the words "paint-skipping worked" to be removed.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8740030 [details]
MozReview Request: Bug 1257641 - Replace the mUpdateScrollOffset bool with an enum, needed in the next patch. r?botond

https://reviewboard.mozilla.org/r/45513/#review42093

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8740031 [details]
MozReview Request: Bug 1257641 - Use empty transactions to carry scroll offset updates to APZ that don't require a repaint. r?mattwoodrow,mstange,botond

https://reviewboard.mozilla.org/r/45515/#review42095

LGTM

::: gfx/layers/Layers.cpp:954
(Diff revision 1)
> +
> +  for (size_t i = 0; i < mScrollMetadata.Length(); i++) {
> +    FrameMetrics& fm = mScrollMetadata[i].GetMetrics();
> +    Maybe<ScrollUpdateInfo> update = Manager()->GetPendingScrollInfoUpdate(fm.GetScrollId());
> +    if (update) {
> +      fm.UpdatePendingScrollInfo(update.value());

Should we clear the map somewhere during the "end transaction" phase? All other pending properties are cleared.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8740032 [details]
MozReview Request: Bug 1257641 - Remove now-unused code for the lightweight scroll offset update message. r?botond

https://reviewboard.mozilla.org/r/45517/#review42097

\o/

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8740033 [details]
MozReview Request: Bug 1257641 - Allow enabling/disabling of displayport suppression for tests. r?botond

https://reviewboard.mozilla.org/r/45519/#review42099

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
(Diff revision 1)
>      if (sActiveSuppressDisplayport == 0 && aShell && aShell->GetRootFrame()) {
>        aShell->GetRootFrame()->SchedulePaint();
>      }
>    }
> -
> -  MOZ_ASSERT(sActiveSuppressDisplayport >= 0);

I'm not a huge fan of allowing sActiveSuppressDisplayport to go into negative numbers.

I think it would be cleaner to have two variables, "are we suppressing right now" (the existing one), and "should we pay attention to the first variable at all" (the one the test wants to set to false).

Comment 17

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

(In reply to Markus Stange [:mstange] from comment #12)
> > +    window.opener.is(utils.checkAndClearPaintedState(elem), !skipping, "Document element didn't get painted; paint-skipping worked");
> 
> I don't think this is conclusive. It's good to check that we didn't
> invalidate anything, but I think this check would pass even without paint
> skipping, assuming we haven't moved the display port. This check checks
> whether we actually ended up invalidating + painting something, but paint
> skipping is about not doing display list building + layer building.
> 
> I think all I'm asking for is for the words "paint-skipping worked" to be
> removed.

Ok, that's fair. I'll remove those words.

(In reply to Botond Ballo [:botond] from comment #14)
> > +      fm.UpdatePendingScrollInfo(update.value());
> 
> Should we clear the map somewhere during the "end transaction" phase? All
> other pending properties are cleared.

Oh yeah, good catch! I'll do it at the end of ApplyPendingUpdatesToSubtree, once the entire tree has been recursed through.

(In reply to Botond Ballo [:botond] from comment #16)
> > -
> > -  MOZ_ASSERT(sActiveSuppressDisplayport >= 0);
> 
> I'm not a huge fan of allowing sActiveSuppressDisplayport to go into
> negative numbers.
> 
> I think it would be cleaner to have two variables, "are we suppressing right
> now" (the existing one), and "should we pay attention to the first variable
> at all" (the one the test wants to set to false).

Ok, I can change that. I thought it would be simpler to just have the one variable but I don't feel strongly about it.

Comment 18

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

Also for the record the try push (including lots of retriggers on the test) seems to be green [1], and a talos push shows a small (~3%) regression in tscrollx [2] but nothing else. I believe the regression is necessary for correctness, and can be deducted from the massive improvement we got from the original landing in bug 1253860. The pushes still have pending jobs so something else may turn up but so far everything looks good.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1d4305f7d14&group_state=expanded
[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=e847cfcb315f&newProject=try&newRevision=4deb2532ac7e&framework=1

Comment 19

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

Comment on attachment 8740030 [details]
MozReview Request: Bug 1257641 - Replace the mUpdateScrollOffset bool with an enum, needed in the next patch. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45513/diff/1-2/

Comment 20

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

Comment on attachment 8740031 [details]
MozReview Request: Bug 1257641 - Use empty transactions to carry scroll offset updates to APZ that don't require a repaint. r?mattwoodrow,mstange,botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45515/diff/1-2/

Comment 21

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

Comment on attachment 8740032 [details]
MozReview Request: Bug 1257641 - Remove now-unused code for the lightweight scroll offset update message. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45517/diff/1-2/

Comment 22

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

Comment on attachment 8740033 [details]
MozReview Request: Bug 1257641 - Allow enabling/disabling of displayport suppression for tests. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45519/diff/1-2/

Comment 23

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

Comment on attachment 8740034 [details]
MozReview Request: Bug 1257641 - Add a test for APZ paint-skipping and event transformations after a skipped paint. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45521/diff/1-2/

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8740031 [details]
MozReview Request: Bug 1257641 - Use empty transactions to carry scroll offset updates to APZ that don't require a repaint. r?mattwoodrow,mstange,botond

https://reviewboard.mozilla.org/r/45515/#review42195

Comment 25

[Botond Ballo [:botond]]

Comment on attachment 8740033 [details]
MozReview Request: Bug 1257641 - Allow enabling/disabling of displayport suppression for tests. r?botond

https://reviewboard.mozilla.org/r/45519/#review42371

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8740034 [details]
MozReview Request: Bug 1257641 - Add a test for APZ paint-skipping and event transformations after a skipped paint. r?botond

https://reviewboard.mozilla.org/r/45521/#review42381

::: gfx/layers/apz/test/mochitest/apz_test_utils.js:101
(Diff revision 2)
>      dump("Flushing APZ repaints was a no-op, triggering callback directly...\n");
>      repaintDone();
>    }
>  }
> +
> +function waitForApzFlushedRepaints(aCallback) {

Can you articulate some guidance on when this should be used, versus just |waitForAllPaints(function() { flushApzRepaints(callback); })|?

::: gfx/layers/apz/test/mochitest/helper_scrollto_tap.html:28
(Diff revision 2)
> +  // directly to the compositor using an empty transaction. We check for this
> +  // by ensuring the document element did not get painted.
> +  var utils = window.opener.SpecialPowers.getDOMWindowUtils(window);
> +  var elem = document.documentElement;
> +  var skipping = location.search == '?true';
> +  utils.checkAndClearPaintedState(elem);

Oh, that's a pretty neat utility! I didn't know we had it.

::: gfx/layers/apz/test/mochitest/helper_scrollto_tap.html:40
(Diff revision 2)
> +  });
> +}
> +
> +function clickButton() {
> +  if (!window.TouchEvent) {
> +    window.opener.ok(true, "Touch events are not supported on this platform, sorry!\n");

Can we use mouse events to produce a "click" in this case?

::: gfx/layers/apz/test/mochitest/test_tap.html:16
(Diff revision 2)
>  
>  // this page just serially loads each one of the following test helper pages in
>  // a new window and waits for it to call testDone()
>  var tests = [
> -  'helper_tap.html',
> +  {'file': 'helper_tap.html'},
> +  // For the following two tests, disable displayport suppression 10 times to

The "10 times" part seems to be a holdover from the previous version of the test.

Comment 27

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

(In reply to Botond Ballo [:botond] from comment #26)
> > +function waitForApzFlushedRepaints(aCallback) {
> 
> Can you articulate some guidance on when this should be used, versus just
> |waitForAllPaints(function() { flushApzRepaints(callback); })|?

Yup, will do.

> ::: gfx/layers/apz/test/mochitest/helper_scrollto_tap.html:28
> > +  utils.checkAndClearPaintedState(elem);
> 
> Oh, that's a pretty neat utility! I didn't know we had it.

Yeah, the reftest harness uses it for no-paint reftests.

> ::: gfx/layers/apz/test/mochitest/helper_scrollto_tap.html:40
> > +    window.opener.ok(true, "Touch events are not supported on this platform, sorry!\n");
> 
> Can we use mouse events to produce a "click" in this case?

Yeah, I suppose that should be ok. It still goes through the event untransformation code which is what I wanted to exercise.

> ::: gfx/layers/apz/test/mochitest/test_tap.html:16
> > +  // For the following two tests, disable displayport suppression 10 times to
> 
> The "10 times" part seems to be a holdover from the previous version of the
> test.

Good catch, I'll fix that up as well.

Comment 28

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)
> Yeah, I suppose that should be ok. It still goes through the event
> untransformation code which is what I wanted to exercise.

Actually that wouldn't help much; the test would still run on the same platforms (Linux and Android) because of the skip-if in mochitest.ini. The window.TouchEvent check is kinda bogus because the test harness forces it on always. Per IRC discussion I'll leave this as-is, and I filed bug 1264017 for adding a test that uses mouse events.

Comment 29

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

I neglected to run this test on Fennec. It mostly passes, but in the paint_skipping=false case (i.e. the "control" case), the check for having painted the document is failing. I'm not entirely sure why, since I definitely see a paint happening (at least, I see a display-list/layer dump at the right time with layout.display-list.dump=true). I'll look a bit more but I might just skip that check in the paint_skipping=false case since it's not really a requirement.

Comment 30

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

Yeah, just because paint skipping is off doesn't mean that the element is guaranteed to get painted, so that check is bogus. On fennec it wasn't hitting the FLB code that sets the painted flag, for example. Presumably this could cause failures later as well if we implement other optimizations that prevent painting, so I modified it to only check the case where paint skipping is on.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f33e7e19f318
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ceca98966d
https://hg.mozilla.org/integration/mozilla-inbound/rev/97daab2fd231
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5232739c23c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdfcc34a189

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f33e7e19f318
https://hg.mozilla.org/mozilla-central/rev/e0ceca98966d
https://hg.mozilla.org/mozilla-central/rev/97daab2fd231
https://hg.mozilla.org/mozilla-central/rev/e5232739c23c
https://hg.mozilla.org/mozilla-central/rev/1bdfcc34a189