Closed Bug 1257641 Opened 4 years ago Closed 4 years ago

Move the scroll offset update message to an empty transaction

Categories

(Core :: Panning and Zooming, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Depends on 2 open bugs)

Details

(Keywords: arch, Whiteboard: [gfx-noted])

Attachments

(5 files)

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.
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"
Assignee: nobody → bugmail.mozilla
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
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.
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.
Attachment #8740031 - Flags: review?(mstange) → review+
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
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 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
Attachment #8740030 - Flags: review?(botond) → review+
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.
Attachment #8740031 - Flags: review?(botond) → review+
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/
Attachment #8740032 - Flags: review?(botond) → review+
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).
Attachment #8740033 - Flags: review?(botond)
(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.
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 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/
Attachment #8740033 - Attachment description: MozReview Request: Bug 1257641 - Allow suppressing of displayport via DOMWindowUtils for tests. r?botond → MozReview Request: Bug 1257641 - Allow enabling/disabling of displayport suppression for tests. r?botond
Attachment #8740033 - Flags: review?(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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45515/diff/1-2/
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 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 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 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
Attachment #8740031 - Flags: review?(matt.woodrow) → review+
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
Attachment #8740033 - Flags: review?(botond) → review+
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.
Attachment #8740034 - Flags: review?(botond) → review+
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
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.
Depends on: 1264933
No longer blocks: 1256727
Duplicate of this bug: 1256727
Depends on: 1328074
You need to log in before you can comment on or make changes to this bug.