Avoid blocking on the main thread to initiate APZ scrollbar dragging

RESOLVED FIXED in Firefox 55

Status

()

Core
Panning and Zooming
RESOLVED FIXED
2 months ago
10 days ago

People

(Reporter: botond, Assigned: botond)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted][qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(16 attachments)

59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
59 bytes, text/x-review-board-request
kats
: review+
Details | Review
(Assignee)

Description

2 months ago
In our current implementation of async scrollbar dragging, we still need to block on the main thread to kick off the drag (once that happens, the rest of the drag will be async).

A further improvement would be to avoid having to block on the main thread at all, such that the entire drag can happen in the compositor, as with other APZ input methods like wheel and touch.
(Assignee)

Updated

2 months ago
Whiteboard: [gfx-noted]
Blocks: 1337841
Priority: -- → P3
Whiteboard: [gfx-noted] → [gfx-noted][qf]
Whiteboard: [gfx-noted][qf] → [gfx-noted][qf:p1]
We will look at this after the async keyboard scrolling (bug 1351783).  Parked until then.
See Also: → bug 1351783
Priority: P3 → --
Given the recent regressions in APZ scrollbar code, we will in fact look into this ahead of the keyboard scrolling, as we're in that code anyway.
Assignee: nobody → botond
(Assignee)

Comment 3

21 days ago
The first thing that occurs to me for this bug is that we can only avoid blocking on the main thread to initiate APZ scrollbar dragging for scroll frames that are already layerized (otherwise, we need to block on the main thread to layerize the scroll frame anyways).

I assume we don't want to change our policy regarding when we layerize scroll frames. (We currently layerize by default the "primary async-scrollable frame", which is the RCD-RSF if it's scrollable, and otherwise the first scrollable subframe that we encounter on the page. In addition, we layerize any scroll when it's scrolled, and keep it layerized until a timer expires.)

One implication of this is that the current codepath, where the main thread kicks off the scrollbar dragging, needs to stick around, and it needs to play nicely with the new codepath that will be added for async initiation of the drag.
(Assignee)

Comment 4

21 days ago
APZ currently waits for the main thread to provide two kinds of information before starting an async scrollbar drag:

  (1) Whether to proceed with the async drag at all. Codewise, this
      is represented in the various early-return conditions in
      nsSliderFrame::StartAPZDrag(), as well as in whether we get
      into that function at all.

  (2) Various metrics needed to perform the async drag, collected
      in AsyncDragMetrics.

Some of the information needed to determine (1), and some of the metrics in (2), are already available on the compositor thread. For the rest, we need to store it in the layer tree, so the information is "already there" when APZ processes a mouse event that can potentially start a drag, and we don't need to wait for the main thread to provide it.

I'm thinking of storing the relevant information on the scroll thumb's ContainerLayer. We already store some similar information (the thumb ratio) there.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

21 days ago
This is a WIP patch series. No functional changes yet, just refactoring and laying the groundwork for future functional changes.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

19 days ago
This patch series is now is now complete, in the sense that all the parts (that I think we'll need) are there.

It passes some basic smoke tests, but I'd like to test it a bit more extensively before requesting review.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce3b916304934bdf5addb09b1ca2d0477989b8cd
(Assignee)

Comment 21

14 days ago
(In reply to Botond Ballo [:botond] from comment #20)
> Try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ce3b916304934bdf5addb09b1ca2d0477989b8cd

This is showing an Android build failure, and two test failures:

  APZCTreeManagerTester.WheelInterruptedByMouseDrag
  test_group_mouseevents
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

14 days ago
(In reply to Botond Ballo [:botond] from comment #21)
> This is showing an Android build failure, and two test failures:
> 
>   APZCTreeManagerTester.WheelInterruptedByMouseDrag
>   test_group_mouseevents

These are all fixed in the updated patch series, along with various other issues found during manual testing.

I'm now satisfied enough with this patch series to put it up for review.

There are a few known limitations of the current implementation:

  1) When initiating an async drag, the APZCTM relies on the position 
     of the thumb in two places

      - during hit testing (to determine whether we hit the thumb 
        rather than another part of the scrollbar)

      - during computation of the drag start offset

     However, the position used for these computations does not take
     into account any async transform that may be applied to the
     thumb as a result of async-scrolling its target APZC (because
     that async transform is only computed and applied in
     AsyncCompositionManager).

  2) We don't handle scroll-to-click, which is a feature of scrollbars
     on some platforms where a mouse-down on a part of the scroll track
     outside the thumb will make the thumb jump to the mouse position,
     and you can drag the thumb from that point in the same gesture.

I plan to fix (1) in a follow-up bug. (2) I will probably put on the backlog.
(Assignee)

Comment 38

14 days ago
(In reply to Botond Ballo [:botond] from comment #37)
>   1) When initiating an async drag, the APZCTM relies on the position 
>      of the thumb in two places
> 
>       - during hit testing (to determine whether we hit the thumb 
>         rather than another part of the scrollbar)
> 
>       - during computation of the drag start offset
> 
>      However, the position used for these computations does not take
>      into account any async transform that may be applied to the
>      thumb as a result of async-scrolling its target APZC (because
>      that async transform is only computed and applied in
>      AsyncCompositionManager).

I forgot to mention the implication of this: if you async-scroll the page (e.g. with mousewheel), and then grab the thumb before the main thread has had a chance to repaint, *that* drag will not be initiated asynchronously.
(Assignee)

Comment 39

14 days ago
New Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d4d9d929ab2e32a04571c55e866837adc9fd652

Comment 40

13 days ago
mozreview-review
Comment on attachment 8864357 [details]
Bug 1349750 - Group scroll thumb-related information in Layer into a ScrollThumbData structure.

https://reviewboard.mozilla.org/r/136018/#review141590
Attachment #8864357 - Flags: review?(bugmail) → review+

Comment 41

13 days ago
mozreview-review
Comment on attachment 8864358 [details]
Bug 1349750 - Remove the no-longer-used WebRenderScrollData[Wrapper]::GetScrollThumbLength().

https://reviewboard.mozilla.org/r/136020/#review141592
Attachment #8864358 - Flags: review?(bugmail) → review+

Comment 42

13 days ago
mozreview-review
Comment on attachment 8864359 [details]
Bug 1349750 - Introduce a PlainOldDataSerializer utility class and use it for SimpleLayerAttributes.

https://reviewboard.mozilla.org/r/136022/#review141596
Attachment #8864359 - Flags: review?(bugmail) → review+

Comment 43

13 days ago
mozreview-review
Comment on attachment 8864360 [details]
Bug 1349750 - Store the entire ScrollThumbData in HitTestingTreeNode.

https://reviewboard.mozilla.org/r/136024/#review141600

Man, once we have IsPod working properly we should use the PlainOldDataSerializer in a lot more places. Forgetting to update the (de-)serializing code on adding new fields is something that has caused numerous bugs.
Attachment #8864360 - Flags: review?(bugmail) → review+

Comment 44

13 days ago
mozreview-review
Comment on attachment 8864361 [details]
Bug 1349750 - Move the scroll thumb length from AsyncDragMetrics to ScrollThumbData.

https://reviewboard.mozilla.org/r/136026/#review141602

::: gfx/layers/LayerAttributes.h:23
(Diff revision 3)
> -    : mDirection(ScrollDirection::NONE), mThumbRatio(0.0f) {}
> -  ScrollThumbData(ScrollDirection aDirection, float aThumbRatio)
> +    : mDirection(ScrollDirection::NONE),
> +      mThumbRatio(0.0f)

nit: line up the comma under the colon

::: gfx/layers/LayerAttributes.h:29
(Diff revision 3)
> +    : mDirection(aDirection),
> +      mThumbRatio(aThumbRatio),
> +      mThumbLength(aThumbLength) {}

ditto
Attachment #8864361 - Flags: review?(bugmail) → review+

Comment 45

13 days ago
mozreview-review
Comment on attachment 8865040 [details]
Bug 1349750 - Move the scroll track extents from AsyncDragMetrics to ScrollThumbData.

https://reviewboard.mozilla.org/r/136692/#review141606
Attachment #8865040 - Flags: review?(bugmail) → review+

Comment 46

13 days ago
mozreview-review
Comment on attachment 8865041 [details]
Bug 1349750 - Add an 'is async-draggable' flag to ScrollThumbData.

https://reviewboard.mozilla.org/r/136694/#review141610

::: layout/xul/nsSliderFrame.cpp:394
(Diff revision 2)
>        if (!scrollFrame) {
>          return;
>        }

You added this early return in the previous patch. I'm wondering if this is ever expected to hit, and if so, if it might affect the display list construction. It might be slightly safer to set isAsyncScrollable=false instead of doing an early return here (and skipping any calculations that require the scrollFrame). I'm not sure if you actually encountered the case where the scrollFrame is null and if it's ok to skip the display list building steps in that case.
Attachment #8865041 - Flags: review?(bugmail) → review+

Comment 47

13 days ago
mozreview-review
Comment on attachment 8865042 [details]
Bug 1349750 - If APZ hit testing hits a scrollbar node, return the node for later use.

https://reviewboard.mozilla.org/r/136696/#review141612
Attachment #8865042 - Flags: review?(bugmail) → review+

Comment 48

13 days ago
mozreview-review
Comment on attachment 8865043 [details]
Bug 1349750 - Have AsyncPanZoomController expose an IsScrollInfoLayer() method.

https://reviewboard.mozilla.org/r/136698/#review141614
Attachment #8865043 - Flags: review?(bugmail) → review+

Comment 49

13 days ago
mozreview-review
Comment on attachment 8865044 [details]
Bug 1349750 - Add HitTestingTreeNode::IsScrollThumbNode().

https://reviewboard.mozilla.org/r/136700/#review141616
Attachment #8865044 - Flags: review?(bugmail) → review+

Comment 50

13 days ago
mozreview-review
Comment on attachment 8865045 [details]
Bug 1349750 - Add a ConvertScrollbarPoint() helper to AsyncPanZoomController.

https://reviewboard.mozilla.org/r/136702/#review141618
Attachment #8865045 - Flags: review?(bugmail) → review+

Comment 51

13 days ago
mozreview-review
Comment on attachment 8865046 [details]
Bug 1349750 - Have AsyncDragMetrics use ScrollDirection instead of rolling its own direction enum.

https://reviewboard.mozilla.org/r/136704/#review141620

Nice cleanup, thanks! :)
Attachment #8865046 - Flags: review?(bugmail) → review+

Comment 52

13 days ago
mozreview-review
Comment on attachment 8866581 [details]
Bug 1349750 - Store the thumb's start offset in ScrollThumbData.

https://reviewboard.mozilla.org/r/138184/#review141622
Attachment #8866581 - Flags: review?(bugmail) → review+

Comment 53

13 days ago
mozreview-review
Comment on attachment 8865047 [details]
Bug 1349750 - Have APZ initiate async scrollbar dragging when possible.

https://reviewboard.mozilla.org/r/136706/#review141624

::: gfx/layers/apz/src/APZCTreeManager.cpp:813
(Diff revision 2)
> +        if (apzDragEnabled && startsDrag && hitScrollbarNode &&
> +            hitScrollbarNode->IsScrollThumbNode() &&
> +            hitScrollbarNode->GetScrollThumbData().mIsAsyncDraggable &&
> +            // check that the scrollbar's target scroll frame is layerized
> +            hitScrollbarNode->GetScrollTargetId() == apzc->GetGuid().mScrollId &&
> +            !apzc->IsScrollInfoLayer() && mInputQueue->GetCurrentDragBlock()) {

That's quite the list of conditions! But it looks good :)
Attachment #8865047 - Flags: review?(bugmail) → review+

Comment 54

13 days ago
mozreview-review
Comment on attachment 8866582 [details]
Bug 1349750 - Put async initiation of scrollbar drags behind a pref.

https://reviewboard.mozilla.org/r/138186/#review141626

Great set of patches, thanks! It was actually fun to review :)
Attachment #8866582 - Flags: review?(bugmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 70

12 days ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #44)
> nit: line up the comma under the colon

Fixed.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #46)
> You added this early return in the previous patch. I'm wondering if this is
> ever expected to hit, and if so, if it might affect the display list
> construction. It might be slightly safer to set isAsyncScrollable=false
> instead of doing an early return here (and skipping any calculations that
> require the scrollFrame). I'm not sure if you actually encountered the case
> where the scrollFrame is null and if it's ok to skip the display list
> building steps in that case.

Good point, to be safe I revised the code as suggested.

The updated patch series is also rebased onto the latest m-c tip.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=422f2874c52bd2829975b5902965b1d86aef17ac
(Assignee)

Comment 71

12 days ago
While testing this patch on a page with artificial jank to see the async initiation in action, I realized that it wasn't actually achieving async initiation. It improved things in that we were no longer waiting for a target confirmation from content, but we still were waiting for a content response.

Thankfully, this is easy to fix. I verified that content can't prevent scrollbar dragging by registering a mouse event listener and preventDefault()ing the event, so we can mark the drag block as having its content response received at the same time we confirm it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 82

12 days ago
mozreview-review
Comment on attachment 8867396 [details]
Bug 1349750 - When APZ initiates an async drag, do not wait for a content response.

https://reviewboard.mozilla.org/r/138940/#review142290
Attachment #8867396 - Flags: review?(bugmail) → review+

Comment 83

11 days ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/649084d26dad
Group scroll thumb-related information in Layer into a ScrollThumbData structure. r=kats
https://hg.mozilla.org/integration/autoland/rev/2383e9189274
Remove the no-longer-used WebRenderScrollData[Wrapper]::GetScrollThumbLength(). r=kats
https://hg.mozilla.org/integration/autoland/rev/c744347a18a5
Introduce a PlainOldDataSerializer utility class and use it for SimpleLayerAttributes. r=kats
https://hg.mozilla.org/integration/autoland/rev/b7f383b496b9
Store the entire ScrollThumbData in HitTestingTreeNode. r=kats
https://hg.mozilla.org/integration/autoland/rev/1c1eb8a23e87
Move the scroll thumb length from AsyncDragMetrics to ScrollThumbData. r=kats
https://hg.mozilla.org/integration/autoland/rev/9b08e1862d70
Add an 'is async-draggable' flag to ScrollThumbData. r=kats
https://hg.mozilla.org/integration/autoland/rev/a243424febf2
Move the scroll track extents from AsyncDragMetrics to ScrollThumbData. r=kats
https://hg.mozilla.org/integration/autoland/rev/1cbbe0c51147
If APZ hit testing hits a scrollbar node, return the node for later use. r=kats
https://hg.mozilla.org/integration/autoland/rev/1fea41757d52
Have AsyncPanZoomController expose an IsScrollInfoLayer() method. r=kats
https://hg.mozilla.org/integration/autoland/rev/aa5b12b989f9
Add HitTestingTreeNode::IsScrollThumbNode(). r=kats
https://hg.mozilla.org/integration/autoland/rev/62d950c3bcca
Add a ConvertScrollbarPoint() helper to AsyncPanZoomController. r=kats
https://hg.mozilla.org/integration/autoland/rev/523311854f7f
Have AsyncDragMetrics use ScrollDirection instead of rolling its own direction enum. r=kats
https://hg.mozilla.org/integration/autoland/rev/eb3028eb3132
Store the thumb's start offset in ScrollThumbData. r=kats
https://hg.mozilla.org/integration/autoland/rev/f20207f554ab
Have APZ initiate async scrollbar dragging when possible. r=kats
https://hg.mozilla.org/integration/autoland/rev/85cd9d40c8e2
Put async initiation of scrollbar drags behind a pref. r=kats
https://hg.mozilla.org/integration/autoland/rev/eee03f85bdad
When APZ initiates an async drag, do not wait for a content response. r=kats
https://hg.mozilla.org/mozilla-central/rev/649084d26dad
https://hg.mozilla.org/mozilla-central/rev/2383e9189274
https://hg.mozilla.org/mozilla-central/rev/c744347a18a5
https://hg.mozilla.org/mozilla-central/rev/b7f383b496b9
https://hg.mozilla.org/mozilla-central/rev/1c1eb8a23e87
https://hg.mozilla.org/mozilla-central/rev/9b08e1862d70
https://hg.mozilla.org/mozilla-central/rev/a243424febf2
https://hg.mozilla.org/mozilla-central/rev/1cbbe0c51147
https://hg.mozilla.org/mozilla-central/rev/1fea41757d52
https://hg.mozilla.org/mozilla-central/rev/aa5b12b989f9
https://hg.mozilla.org/mozilla-central/rev/62d950c3bcca
https://hg.mozilla.org/mozilla-central/rev/523311854f7f
https://hg.mozilla.org/mozilla-central/rev/eb3028eb3132
https://hg.mozilla.org/mozilla-central/rev/f20207f554ab
https://hg.mozilla.org/mozilla-central/rev/85cd9d40c8e2
https://hg.mozilla.org/mozilla-central/rev/eee03f85bdad
Status: NEW → RESOLVED
Last Resolved: 10 days ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.