Closed Bug 1349750 Opened 7 years ago Closed 7 years ago

APZ: Avoid blocking on the main thread to initiate APZ scrollbar dragging

Categories

(Core :: Panning and Zooming, enhancement)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: botond, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(16 files)

Bug 1349750 - Group scroll thumb-related information in Layer into a ScrollThumbData structure.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Remove the no-longer-used WebRenderScrollData[Wrapper]::GetScrollThumbLength().
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Introduce a PlainOldDataSerializer utility class and use it for SimpleLayerAttributes.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Store the entire ScrollThumbData in HitTestingTreeNode.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Move the scroll thumb length from AsyncDragMetrics to ScrollThumbData.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Move the scroll track extents from AsyncDragMetrics to ScrollThumbData.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Add an 'is async-draggable' flag to ScrollThumbData.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - If APZ hit testing hits a scrollbar node, return the node for later use.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Have AsyncPanZoomController expose an IsScrollInfoLayer() method.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Add HitTestingTreeNode::IsScrollThumbNode().
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Add a ConvertScrollbarPoint() helper to AsyncPanZoomController.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Have AsyncDragMetrics use ScrollDirection instead of rolling its own direction enum.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Have APZ initiate async scrollbar dragging when possible.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Store the thumb's start offset in ScrollThumbData.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - Put async initiation of scrollbar drags behind a pref.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1349750 - When APZ initiates an async drag, do not wait for a content response.
59 bytes, text/x-review-board-request
kats
: review+
Details 
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.
Whiteboard: [gfx-noted]
Blocks: QuantumFlow
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: → apz-keyboard
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
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.
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.
This is a WIP patch series. No functional changes yet, just refactoring and laying the groundwork for future functional changes.
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
(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
(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.
(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.
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 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 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 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 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 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 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 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 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 on attachment 8865044 [details]
Bug 1349750 - Add HitTestingTreeNode::IsScrollThumbNode().

https://reviewboard.mozilla.org/r/136700/#review141616
Attachment #8865044 - Flags: review?(bugmail) → 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 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 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 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 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+
(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
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 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+
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
Depends on: 1368315
There's something very wrong about the scrollbar.

If I click an empty space on the scrollbar and hold the button, the thumb doesn't stop at that point. It overshoots it.
With default settings it only overshoots by abou the height of the thumb


But it's a lot worse if you have have the scrolling settings customized like have. This time the position is overshot by a ridiculous amount. Might be roughly as much as the the starting distance between the thumb and the clicked position. So it might be half a page or more.


general.smoothScroll.lines.durationMaxMS;450
general.smoothScroll.lines.durationMinMS;450
general.smoothScroll.mouseWheel.durationMaxMS;1500
general.smoothScroll.pages.durationMaxMS;385
general.smoothScroll.pages.durationMinMS;385

(I posted all my customized settings for completeness' sake)


Naturally I have these customized since with default it doesn't feel smooth at all, especially the scrollbar press and hold scrolling.
Flags: needinfo?(botond)
avada, what OS are you testing on? 

On Linux, I see two behaviours when clicking on empty space in the scrollbar, on Firefox Release:

  1) Plain left click ==> The thumb immediately jumps to the clicked position
 
  2) Shift + left click, or right click ==> The thumb starts moving in the
     direction of your click, but if you keep holding down it doesn't stop
     until it reaches the end of the scroll range.

I am seeing both of these behaviours preserved in Firefox Nightly.
(In reply to Botond Ballo [:botond] from comment #86)
> avada, what OS are you testing on? 
> 
> On Linux, I see two behaviours when clicking on empty space in the
> scrollbar, on Firefox Release:
> 
>   1) Plain left click ==> The thumb immediately jumps to the clicked position
>  
>   2) Shift + left click, or right click ==> The thumb starts moving in the
>      direction of your click, but if you keep holding down it doesn't stop
>      until it reaches the end of the scroll range.
> 
> I am seeing both of these behaviours preserved in Firefox Nightly.

I'm on Windows (8.1). And as far as I remember it never worked like that. Right click does nothing at all.
Left click normally scrolls by one screen height and on hold keeps doing it until it reaches the cursor, or at least it should, but it overshoots.
Shift + leftclick (which I didn't know about until now) jumps instantly to the cursor position. (Accurately)

Here are some screencasts showing the behavior with default and customized general.smoothScroll.pages.* values:


https://drive.google.com/open?id=0ByfdfPvnoDuzSFpNT0VRdVRpUFk
(In reply to avada from comment #87)
> I'm on Windows (8.1). And as far as I remember it never worked like that.
> Right click does nothing at all.
> Left click normally scrolls by one screen height and on hold keeps doing it
> until it reaches the cursor, or at least it should, but it overshoots.
> Shift + leftclick (which I didn't know about until now) jumps instantly to
> the cursor position. (Accurately)

Yeah, some of this behaviour varies by platform.

I'll be near a Windows machine on Friday, I'll test the behaviour on Windows then. Leaving needinfo on me until then.
(In reply to Botond Ballo [:botond] from comment #88)
> I'll be near a Windows machine on Friday, I'll test the behaviour on Windows
> then. Leaving needinfo on me until then.

I can reproduce the overshooting described in comment 85 on Windows. However, it doesn't seem to be related to the changes in this bug, and in fact it doesn't seem to be a recent regression at all. Mozregression gave me a regression window in August 2014.
Flags: needinfo?(botond)
(In reply to Botond Ballo [:botond] from comment #89)
> I can reproduce the overshooting described in comment 85 on Windows.
> However, it doesn't seem to be related to the changes in this bug, and in
> fact it doesn't seem to be a recent regression at all. Mozregression gave me
> a regression window in August 2014.

Filed bug 1369847 for this.
(In reply to Botond Ballo [:botond] from comment #89)
> 
> I can reproduce the overshooting described in comment 85 on Windows.
> However, it doesn't seem to be related to the changes in this bug, and in
> fact it doesn't seem to be a recent regression at all. Mozregression gave me
> a regression window in August 2014.

I see. I wasn't sure how old it is, I thought I'd mention it anyway since you were touching scrollbar code.
Alias: apz-scrollbar-dragging
Summary: Avoid blocking on the main thread to initiate APZ scrollbar dragging → APZ: Avoid blocking on the main thread to initiate APZ scrollbar dragging
I think it's misleading to alias this bug as "apz-scrollbar-dragging". The bug that tracks APZ scrollbar dragging is bug 1211610. This bug concerns avoiding the initial round-trip to the main thread to kick off APZ scrollbar dragging, which is just a piece of the puzzle.
Alias: apz-scrollbar-dragging
Depends on: 1402995
Performance Impact: --- → P1
Whiteboard: [gfx-noted][qf:p1] → [gfx-noted]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: