Closed Bug 1219296 Opened 9 years ago Closed 8 years ago

Ship snap points to the compositor so that we can snap asynchronously

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: kats, Assigned: botond)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(12 files, 6 obsolete files)

MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats
(deleted), text/x-review-board-request
kats
: review+
ritu
: approval-mozilla-beta+
Details
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats
(deleted), text/x-review-board-request
kats
: review+
Details
Local microbenchmark results
(deleted), image/png
Details
Results broken down by patch in the patch series
(deleted), image/png
Details 
The current implementation of the CSS snap points lives on the main thread. When APZ does a scroll, it requests that the main thread figure out the appropriate snap point and trigger a smooth scroll to that destination. This works ok for touch-based panning in APZ, because we can estimate where the scroll will end up based on the velocity and physics at the time the finger is lifted. For trackpad-based wheel events also this is doable.

However for "clicky" scroll wheel events the main-thread implementation simply smooth-scrolls to the next/prev snap point for each roll of the wheel, as described at [1]. Doing this in the compositor requires the compositor to know the next/prev snap points.

Additionally, in some cases, waiting on the main thread to determine the snap point is too slow, resulting in visual glitching or jankiness. Cwiiis reported seeing such anomalies on B2G which is starting to make extensive use of snap points in both the homescreen and the app switcher.

I think eventually we should try and send snap point information to the compositor if possible, so that snapping can be done entirely without waiting on the main-thread, except for degenerate cases.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1141884#c20
Whiteboard: [gfx-noted]
Blocks: 1177842
Blocks: 1228820
No longer blocks: 1141884
I'm going to try implementing this, to make progress on bug 1249040.
Assignee: nobody → botond
I've had a look at how main thread scroll snapping works. 

It doesn't maintain a list of absolute snap points for the scroll frame that could be shipped to the compositor as is; rather, whenever a scroll that should result in scroll snapping occurs, a set of candidate snap points that are nearby to the original scroll destination is constructed on demand, with the following information as inputs:

  (a) The scroll frame's scroll-snap-type, scroll-snap-destination,
      scroll-snap-points, and scroll range.

  (b) Any scroll-snap-coordinates defined on frames in the scroll
      frame's subtree (used in conjunction with layout metrics for
      the relevant frame).

  (c) The start position and original (without snapping) destination
      for the current scroll operation.

It then chooses the best snap point from the candidates (this really happens while building the candidates, too).

Given that, I can envision two approaches to shipping snap points to the compositor:

  1) Ship the inputs listed in (a) and (b) to the compositor, and have
     the compositor compute snap points on demand from these inputs
     (plus (c) which it knows).

     This would involve:

       - Creating a representation for (a) and (b) that can be
         shipped to the compositor. For (a) this is pretty 
         straightforward. For (b), I believe we can collapse the 
         "any scroll-snap-coordinates ... in conjunction with layout 
         metrics" into a single value per scroll-snap-coordinate.

       - Reformulating the algorithm for choosing candidate snap
         points (and choosing the best candidate) to depend only
         on this new representation, and not explicitly on content-
         side data structures (like the frame tree), so that it
         can be run on the compositor side.

     The compositor should then be able to accurately choose a
     snap point and snap to it.

  2) Keep the inputs and the snap point computation algorithm on
     the main thread, but run the algorithm "speculatively" when
     sending a layers transaction, to compute a few snap points 
     to either side of the current scroll offset, and ship those 
     pre-computed snap points to the compositor.

     The compositor should then be able to snap accurately if the
     original scroll destination lies within the area "covered"
     by the pre-computed snap points, and would have to defer to
     the main thread otherwise.

     This would involve some modifications to the algorithm for
     computing snap points, so it doesn't necessarily require a
     concrete known scroll destination.

I'm leaning towards approach (1), because it produces the better (more accurate) outcome, and it doesn't seem like it would be significantly trickier than (2). However, I'm open to input on this question.
Kip in particular might have some valuable insights about which approach is more promising.
Flags: needinfo?(kgilbert)
Finally, it's worth noting that there's a proposal at the W3C to revamp how scroll snapping works [1]. I'm not sure what our plans are for implementing it, but it may be worth considering when choosing our approach here. 

In particular, since the new spec changes how snap points are computed, if we go with approach (1) then implementing the new spec will require changes to the information that we make available to the compositor; if we go with approach (2), the protocol with the compositor (sending pre-computed snap points) would remain the same, and only the algorithm to compute those snap points on the main thread would change.

[1] https://drafts.csswg.org/css-scroll-snap/
The css-scroll-snap spec has already been resolved as the way forward in the CSSWG. (The css-snappoints editor has been slow to implement the change.)
(In reply to Botond Ballo [:botond] [C++ standards meeting Feb 29 - Mar 5] from comment #2)
> I've had a look at how main thread scroll snapping works. 
> 
> It doesn't maintain a list of absolute snap points for the scroll frame that
> could be shipped to the compositor as is; rather, whenever a scroll that
> should result in scroll snapping occurs, a set of candidate snap points that
> are nearby to the original scroll destination is constructed on demand, with
> the following information as inputs:
> 
>   (a) The scroll frame's scroll-snap-type, scroll-snap-destination,
>       scroll-snap-points, and scroll range.
> 
>   (b) Any scroll-snap-coordinates defined on frames in the scroll
>       frame's subtree (used in conjunction with layout metrics for
>       the relevant frame).
> 
>   (c) The start position and original (without snapping) destination
>       for the current scroll operation.
> 
> It then chooses the best snap point from the candidates (this really happens
> while building the candidates, too).
> 
> Given that, I can envision two approaches to shipping snap points to the
> compositor:
> 
>   1) Ship the inputs listed in (a) and (b) to the compositor, and have
>      the compositor compute snap points on demand from these inputs
>      (plus (c) which it knows).
> 
>      This would involve:
> 
>        - Creating a representation for (a) and (b) that can be
>          shipped to the compositor. For (a) this is pretty 
>          straightforward. For (b), I believe we can collapse the 
>          "any scroll-snap-coordinates ... in conjunction with layout 
>          metrics" into a single value per scroll-snap-coordinate.
> 
>        - Reformulating the algorithm for choosing candidate snap
>          points (and choosing the best candidate) to depend only
>          on this new representation, and not explicitly on content-
>          side data structures (like the frame tree), so that it
>          can be run on the compositor side.
> 
>      The compositor should then be able to accurately choose a
>      snap point and snap to it.
> 
>   2) Keep the inputs and the snap point computation algorithm on
>      the main thread, but run the algorithm "speculatively" when
>      sending a layers transaction, to compute a few snap points 
>      to either side of the current scroll offset, and ship those 
>      pre-computed snap points to the compositor.
> 
>      The compositor should then be able to snap accurately if the
>      original scroll destination lies within the area "covered"
>      by the pre-computed snap points, and would have to defer to
>      the main thread otherwise.
> 
>      This would involve some modifications to the algorithm for
>      computing snap points, so it doesn't necessarily require a
>      concrete known scroll destination.
> 
> I'm leaning towards approach (1), because it produces the better (more
> accurate) outcome, and it doesn't seem like it would be significantly
> trickier than (2). However, I'm open to input on this question.

These are both fine approaches; however, if #1 is not significantly more complex I would recommend it as it would indeed provide better results.  There were cases, especially on mobile, where snap points were determined too late into the scroll due to main thread activity and the resulting snap was abrupt.  This should be solved with the #1 approach.
Flags: needinfo?(kgilbert)
Based on comment 6 and my own leaning, I will pursue approach #1.
If we're going to be shipping snap point information to the compositor, the FrameMetrics is a logical place to store it.

However, FrameMetrics is a very large structure already, and particularly for uses such as sending it back to content in repaint requests, this would just add unnecessary bloat.

To avoid this, I propose splitting the fields not needed for lighter-weight uses such as repaint requests, out of FrameMetrics, and into a more general ScrollMetadata structure on the layer.

Attached is a WIP patch for doing this. It doesn't compile yet.
Here's an updated patch that compiles and seems to work (at the "doesn't immediately crash the browser" level). I'd like to exercise it a bit more before posting it for review.
Attachment #8727685 - Attachment is obsolete: true
Very much a WIP.
While working on this, I realized that the main-thread scroll snap implementation only uses APZ to scroll to the destination if the pref "layout.css.scroll-behavior.enabled" is set to true; if it's set to false, we use a main-thread animation (ScrollFrameHelper::AsyncScroll) instead.

Kip, is this interaction between scroll snapping and scroll-behavior governed by the spec? Is it important for APZ to replicate it (i.e. to use main-thread scrolling to perform scroll snapping if "layout.css.scroll-behavior.enabled" is set to false)?
Flags: needinfo?(kgilbert)
Still a WIP, but starting to take shape.
Attachment #8728739 - Attachment is obsolete: true
Attachment #8729773 - Attachment is obsolete: true
It's probably follow-up, but I suggest that the behaviour for snap points should act like magnetism - whereby every snap point has a certain attract force, and once the momentum of the fling is too low, it won't escape this force. This will give the sensation to the user of being able to 'feel' each snap point, as well as be consistent with our overscroll oscillation bounce effect. It would be a really nice bit of polish that would put us ahead of other browsers' behaviour here.
Comment on attachment 8731505 [details]
MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/1-2/
Comment on attachment 8731506 [details]
MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/1-2/
Attachment #8731504 - Attachment description: MozReview Request: [WIP] Bug 1219296 - Ship snap point information to the compositor. r=kats → MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/1-2/
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/1-2/
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/1-2/
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/1-2/
Attachment #8731509 - Attachment description: MozReview Request: [WIP] Bug 1219296 - APZ support for scroll snapping without going through the main thread. r=kats → MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats
Here's an updated patch series. The patches are now "feature-complete", but they need testing. (I've done some preliminary testing and fixed some crashes, but I haven't verified yet that APZ is actually driving smooth scrolling as intended.)
So APZ isn't hooked up to perform scroll snapping in response to wheel scrolling; we can hook that up that in a follow-up if we'd like. I tested it with touch scrolling on my laptop, and basic use cases seem to work, but there is some weird behaviour near the edge of a scrollframe, and some weird behaviour related to handoff, that I need to investigate more.
Attachment #8731503 - Flags: review?(bugmail.mozilla)
Comment on attachment 8731503 [details]
MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40657/diff/1-2/
Attachment #8732462 - Flags: review?(bugmail.mozilla)
Comment on attachment 8732462 [details]
MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/1-2/
Comment on attachment 8731505 [details]
MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/2-3/
Attachment #8731505 - Flags: review?(bugmail.mozilla)
Comment on attachment 8731506 [details]
MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/2-3/
Attachment #8731506 - Flags: review?(bugmail.mozilla)
Comment on attachment 8732463 [details]
MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/1-2/
Attachment #8732463 - Flags: review?(bugmail.mozilla)
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/2-3/
Attachment #8731504 - Flags: review?(bugmail.mozilla)
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/2-3/
Attachment #8731507 - Flags: review?(bugmail.mozilla)
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/2-3/
Attachment #8731508 - Flags: review?(bugmail.mozilla)
Attachment #8731509 - Flags: review?(bugmail.mozilla)
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/2-3/
Attachment #8732464 - Attachment is obsolete: true
The updated patch series fixes the bugs I found during testing.

Markus also pointed out that we can avoid sending nsStyleCoord over IPC altogether, by moving some of the processing that doesn't depend on the destination of a particular scroll, from GetSnapPointForDestination() into ComputeScrollSnapInfo(). The updated patches implement this improvement. (Thanks, Markus!)

Posted patch series for review.
Comment on attachment 8732462 [details]
MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/2-3/
Comment on attachment 8731505 [details]
MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/3-4/
Comment on attachment 8731506 [details]
MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/3-4/
Comment on attachment 8732463 [details]
MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/2-3/
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/3-4/
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/3-4/
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/3-4/
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/3-4/
(Fixed a patch-splitting error.)
Comment on attachment 8731503 [details]
MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats

https://reviewboard.mozilla.org/r/40657/#review38165

::: gfx/layers/FrameMetrics.cpp:13
(Diff revision 2)
>  
>  namespace mozilla {
>  namespace layers {
>  
>  const FrameMetrics::ViewID FrameMetrics::NULL_SCROLL_ID = 0;
>  const FrameMetrics FrameMetrics::sNullMetrics;

Can we get rid of the sNullMetrics now?

::: gfx/layers/Layers.h:880
(Diff revision 2)
>     * See the documentation in LayerMetricsWrapper.h for a more detailed
>     * explanation of this conceptual equivalence.
>     *
>     * Note also that there is actually a many-to-many relationship between
> -   * Layers and FrameMetrics, because multiple Layers may have identical
> -   * FrameMetrics objects. This happens when those layers belong to the
> +   * Layers and ScrollMetadata, because multiple Layers may have identical
> +   * ScrollMetadataobjects. This happens when those layers belong to the

space before "objects"

::: gfx/layers/apz/src/AsyncPanZoomController.h:185
(Diff revision 2)
>    /**
> -   * A shadow layer update has arrived. |aLayerMetrics| is the new FrameMetrics
> +   * A shadow layer update has arrived. |aScrollMetdata| is the new ScrollMetadata
>     * for the container layer corresponding to this APZC.
>     * |aIsFirstPaint| is a flag passed from the shadow
> -   * layers code indicating that the frame metrics being sent with this call are
> -   * the initial metrics and the initial paint of the frame has just happened.
> +   * layers code indicating that the scroll metadata being sent with this call are
> +   * the initial ,etadata and the initial paint of the frame has just happened.

typo
Attachment #8731503 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8732462 [details]
MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats

https://reviewboard.mozilla.org/r/41197/#review38187
Attachment #8732462 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8731505 [details]
MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats

https://reviewboard.mozilla.org/r/40661/#review38189
Attachment #8731505 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #53)
> There's build failures, btw:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fdbddd742ac
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dd576e7887d

Sorry about that! The cause is that gcc is strict about method names (in this case, LayerMetricsWrapper::ScrollMetadata()) that clash with class names (in this case, ScrollMetadata), while clang (which I was building with locally) is permissive about it.

The fix is to rename LayerMetricsWrapper::ScrollMetadata().
Attachment #8731506 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8731506 [details]
MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats

https://reviewboard.mozilla.org/r/40663/#review38465

::: layout/generic/ScrollSnap.cpp:311
(Diff revision 4)
> +    snapped = true;
> +  }
> +  return snapped ? Some(finalPos) : Nothing();
> +}
> +
> +}

// namespace mozilla

::: layout/generic/nsGfxScrollFrame.cpp:5771
(Diff revision 4)
> -    snapped = true;
> -  }
> -  if (snapped) {
> -    aDestination = finalPos;
>    }
> -  return snapped;
> +  return snapPoint.isSome();

I'd prefer a |return true;| inside the if and a |return false;| here.

Also I'm assuming that you didn't modify any code during the move - MozReview doesn't flag it as "moved from line X" but I didn't go over it line-by-line
Comment on attachment 8732463 [details]
MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats

https://reviewboard.mozilla.org/r/41199/#review38467

::: gfx/thebes/gfxPrefs.h:397
(Diff revision 3)
>    DECL_GFX_PREF(Live, "layers.single-tile.enabled",            LayersSingleTileEnabled, bool, true);
>  
>    DECL_GFX_PREF(Live, "layout.css.scroll-behavior.damping-ratio", ScrollBehaviorDampingRatio, float, 1.0f);
>    DECL_GFX_PREF(Live, "layout.css.scroll-behavior.enabled",    ScrollBehaviorEnabled, bool, false);
>    DECL_GFX_PREF(Live, "layout.css.scroll-behavior.spring-constant", ScrollBehaviorSpringConstant, float, 250.0f);
> +  DECL_GFX_PREF(Live, "layout.css.scroll-snap.proximity-threshold", ScrollSnapProximityThreshold, int32_t, 200);

Move this down two lines so it's alpha-sorted

::: layout/generic/ScrollSnap.cpp:10
(Diff revision 3)
>  
>  #include "FrameMetrics.h"
>  #include "ScrollSnap.h"
>  #include "mozilla/Maybe.h"
>  #include "mozilla/Preferences.h"
> +#include "gfxPrefs.h"

Move this up two lines so it's alpha sorted :)

I'm assuming you're sticking with your "uppercase first" alpha ordering here.
Attachment #8732463 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

https://reviewboard.mozilla.org/r/40659/#review38471
Attachment #8731504 - Flags: review?(bugmail.mozilla) → review+
Attachment #8731507 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

https://reviewboard.mozilla.org/r/40665/#review38473
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

https://reviewboard.mozilla.org/r/40667/#review38477
Attachment #8731508 - Flags: review?(bugmail.mozilla) → review+
https://reviewboard.mozilla.org/r/40663/#review38485

::: layout/generic/ScrollSnap.h:26
(Diff revision 4)
> +   * |aSnapInfo|, |aScrollPortSize|, and |aScrollRange| are characteristics
> +   * of the scroll frame for which snapping is being performed.
> +   * If a suitable snap point could be found, it is returned. Otherwise, an
> +   * empty Maybe is returned.
> +   */
> +  static Maybe<nsPoint> GetSnapPointForDestination(

Add a big scary warning here that this function may be used off-main-thread and should not do anything that touches main-thread-only structures without appropriate locking.
Attachment #8731509 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

https://reviewboard.mozilla.org/r/40669/#review38487
Nice work! This seems like a pretty clean implementation overall :)

Before this lands I'd like to make sure there are no significant talos regressions; I'm a little worried about the overhead of running the snap calculations even though it's likely not very significant. Please do a try push with talos and talos-e10s on linux (at least) to make sure. Thanks!
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #51)
> >  const FrameMetrics FrameMetrics::sNullMetrics;
> 
> Can we get rid of the sNullMetrics now?

Good idea. I replaced its remaining uses is LayerMetricsWrapper with sNullMetadata.GetMetrics().

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> Also I'm assuming that you didn't modify any code during the move -
> MozReview doesn't flag it as "moved from line X" but I didn't go over it
> line-by-line

The body of GetSnapPointForDestination() was adjusted to get information about the scroll frame from its parameters, rather than from the fields of ScrollFrameHelper (which it was previously a method of). CalcSnapPoints and SnappingEdgeCallback were copied over verbatim.
(In reply to Botond Ballo [:botond] from comment #11)
> While working on this, I realized that the main-thread scroll snap
> implementation only uses APZ to scroll to the destination if the pref
> "layout.css.scroll-behavior.enabled" is set to true; if it's set to false,
> we use a main-thread animation (ScrollFrameHelper::AsyncScroll) instead.
> 
> Kip, is this interaction between scroll snapping and scroll-behavior
> governed by the spec? Is it important for APZ to replicate it (i.e. to use
> main-thread scrolling to perform scroll snapping if
> "layout.css.scroll-behavior.enabled" is set to false)?

In the current patch series, APZ does not replicate this behaviour; all APZ-triggered scroll snaps result in APZ smooth scrolls. If you feel strongly that this shouldn't be the case, let me know.
Flags: needinfo?(kgilbert)
(In reply to Botond Ballo [:botond] from comment #65)
> Try push, including Talos tests:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=539ce0c92fc6

There's bustage in Android-only code, which I've fixed locally.

The debug build is also showing nsTArray leaks. I believe this is caused by the patch adding an nsTArray field to the global sNullMetadata object; we'll need to change that to use StaticAutoPtr instead.
Comment on attachment 8731503 [details]
MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40657/diff/2-3/
Comment on attachment 8732462 [details]
MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/3-4/
Comment on attachment 8731505 [details]
MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/4-5/
Comment on attachment 8731506 [details]
MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/4-5/
Comment on attachment 8732463 [details]
MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/3-4/
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/4-5/
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/4-5/
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/4-5/
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/4-5/
Comment on attachment 8734110 [details]
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/1-2/
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/5-6/
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/5-6/
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/5-6/
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/5-6/
Comment on attachment 8734110 [details]
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats

https://reviewboard.mozilla.org/r/42091/#review38629
Attachment #8734110 - Flags: review?(bugmail.mozilla) → review+
Whoops! We can't initialize sNullMetadata in AsyncPanZoomController::InitializeGlobalState(), because that only gets called if APZ is enabled, while there is some code (LayerHasCheckerboardingAPZC()) that tries to access sNullMetadata even if APZ is disabled.
Initializing it in the CompositorParent constructor instead:

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

This time, I'm going to wait for a clean Try push before requesting re-review :)
Of course, there's no CompositorParent in gtests. (No one calls AsyncPanZoomController::InitializeGlobalState(), either, but the gtests don't seem to need the things that sets up.)
(In reply to Botond Ballo [:botond] from comment #84)
> (LayerHasCheckerboardingAPZC()) that tries to access sNullMetadata even if
> APZ is disabled.

We can probably guard this function with an APZ-enabled check.
Blocks: 1259296
(In reply to Botond Ballo [:botond] from comment #31)
> So APZ isn't hooked up to perform scroll snapping in response to wheel
> scrolling; we can hook that up that in a follow-up if we'd like.

We definitely want to do this (among other things, it's required to fix bug 1249040). I filed bug 1259296 for it.
Blocks: 1259298
(In reply to Chris Lord [:cwiiis] from comment #20)
> It's probably follow-up, but I suggest that the behaviour for snap points
> should act like magnetism - whereby every snap point has a certain attract
> force, and once the momentum of the fling is too low, it won't escape this
> force. This will give the sensation to the user of being able to 'feel' each
> snap point, as well as be consistent with our overscroll oscillation bounce
> effect. It would be a really nice bit of polish that would put us ahead of
> other browsers' behaviour here.

Thanks for the suggestion! Filed bug 1259298 for this.
Blocks: 1259301
(In reply to Botond Ballo [:botond] from comment #86)
> Of course, there's no CompositorParent in gtests. (No one calls
> AsyncPanZoomController::InitializeGlobalState(), either, but the gtests
> don't seem to need the things that sets up.)

I fixed the APZ gtests, but it turns out LayerMetricsWrapper has its own gtests in TestLayers.cpp. This StaticAutoPtr thing is turning out to be a lot more trouble than I imagined...
Comment on attachment 8734110 [details]
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/2-3/
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/6-7/
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/6-7/
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/6-7/
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/6-7/
All right, this Try push [1] shows everything except gtests passing, and this one [2] shows gtests passing (with the only code changes between the two being to gtest files).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=248686502573
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=923412c1afb4
Comment on attachment 8734110 [details]
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats

Asking for re-review on this because I had to make a number of changes to get tests to pass.

I'm not thrilled about having to do all this to get the StaticAutoPtr to be alive at the right times. If you have suggestions for better ways to do this, let me know.
Attachment #8734110 - Flags: review+ → review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #87)
> (In reply to Botond Ballo [:botond] from comment #84)
> > (LayerHasCheckerboardingAPZC()) that tries to access sNullMetadata even if
> > APZ is disabled.
> 
> We can probably guard this function with an APZ-enabled check.

I already had the other solution (using the CompositorParent constructor) implemented and running on Try when I saw this, so I stuck to it. Let me know if you prefer doing this instead, I can change it.

(Note that this is orthogonal to what we do in the gtests.)
(In reply to Botond Ballo [:botond] from comment #98)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #87)
> > (In reply to Botond Ballo [:botond] from comment #84)
> > > (LayerHasCheckerboardingAPZC()) that tries to access sNullMetadata even if
> > > APZ is disabled.
> > 
> > We can probably guard this function with an APZ-enabled check.
> 
> I already had the other solution (using the CompositorParent constructor)
> implemented and running on Try when I saw this, so I stuck to it. Let me
> know if you prefer doing this instead, I can change it.

I kinda do prefer guarding LayerHasCheckerboardingAPZC. For one it will avoid unnecessary walking around in the tree when APZ is disabled, and it simplifies the startup code a lot. I don't particularly like having APZ-specific init stuff in CompositorParent.

> (Note that this is orthogonal to what we do in the gtests.)

I'm not sure what this means. AFAIK all our gtests create an APZCTreeManager, so they will all be calling AsyncPanZoomController::InitializeGlobalState. If we put the initialization of the StaticAutoPtr there we don't need a lot of the gtest changes you had to make in this last iteration (or maybe I'm misreading the interdiffs?).
Comment on attachment 8734110 [details]
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats

https://reviewboard.mozilla.org/r/42091/#review38733

Dropping review flag. As commented above, I'm fine with making this a StaticAutoPtr, but I think initializing it in InitializeGlobalState is the way to go. In the long term we'll have APZ enabled everywhere so it being uninitialized shouldn't be a problem.
Attachment #8734110 - Flags: review?(bugmail.mozilla)
(In reply to (Back on Mar30) Kartikaya Gupta (email:kats@mozilla.com) from comment #99)
> > (Note that this is orthogonal to what we do in the gtests.)
> 
> I'm not sure what this means. AFAIK all our gtests create an
> APZCTreeManager, so they will all be calling
> AsyncPanZoomController::InitializeGlobalState. If we put the initialization
> of the StaticAutoPtr there we don't need a lot of the gtest changes you had
> to make in this last iteration (or maybe I'm misreading the interdiffs?).

You're right in that we shouldn't need the changes to APZCTreeManagerTester. We'll still need the change to TestLayers.cpp. I agree that it's an improvement overall.

Try push with these changes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d660faf4ad7
(In reply to Botond Ballo [:botond] from comment #101)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d660faf4ad7

Unfortunately, LayerHasCheckerboardingAPZC() wasn't the only thing doing a LayerMetricsWrapper tree walk with APZ disabled. AlignFixedAndStickyLayers() does it too.
Here's a hacky workaround for the crash in AlignFixedAndStickyLayers(). With this, I have a green Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69159359a46a

However, I don't like this, because it muddies the interface of LayerMetricsWrapper. (Under what circumstances is it safe to call Metrics() or Metadata() without checking HasScrollMetadata() first?)

To me, the fact that AlignFixedAndStickyLayers() is legitimately used in non-APZ setups, and it does a LayerMetricsWrapper tree walk, suggests that initializing things needed by LayerMetricsWrapper, such as sNullMetadata, in code that's not APZ-specific (such as CompositorParent (these days called CompositorBridgeParent)), would be reasonable.

However, I'm open to alternative suggestions.
Attachment #8736074 - Flags: feedback?(bugmail.mozilla)
Attachment #8736074 - Attachment is patch: true
Comment on attachment 8736074 [details] [diff] [review]
Hacky fix for crash in AlignFixedAndStickyLayers()

Review of attachment 8736074 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Botond Ballo [:botond] from comment #103)
> To me, the fact that AlignFixedAndStickyLayers() is legitimately used in
> non-APZ setups, and it does a LayerMetricsWrapper tree walk, suggests that
> initializing things needed by LayerMetricsWrapper, such as sNullMetadata, in
> code that's not APZ-specific (such as CompositorParent (these days called
> CompositorBridgeParent)), would be reasonable.

Yeah, you're right. If we're using the sNullMetadata in non-APZ-specific code then we shouldn't initialize it in APZ-specific code. I didn't realize it was so widely used. In that case initializing it from CompositorBridgeParent or (if possible) gfxPlatform should be ok.
Attachment #8736074 - Flags: feedback?(bugmail.mozilla)
So it turns out gtests have more things going for them than I orginally thought - they've got XPCOM, and they can handle called gfxPlatform::Init() as well. That made implementing the approach we discussed on IRC fairly simple.
Comment on attachment 8731503 [details]
MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40657/diff/3-4/
Attachment #8734110 - Flags: review?(bugmail.mozilla)
Comment on attachment 8732462 [details]
MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/4-5/
Comment on attachment 8731505 [details]
MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/5-6/
Comment on attachment 8731506 [details]
MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/5-6/
Comment on attachment 8732463 [details]
MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/4-5/
Comment on attachment 8734110 [details]
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/3-4/
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/7-8/
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/7-8/
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/7-8/
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/7-8/
Attachment #8736074 - Attachment is obsolete: true
And here's a Try push without the patches to get baseline Talos results:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eaf176c9d83
Comment on attachment 8734110 [details]
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats

https://reviewboard.mozilla.org/r/42091/#review40059

::: gfx/layers/composite/ContainerLayerComposite.cpp:364
(Diff revision 4)
>  
>  static bool
> -NeedToDrawCheckerboardingForLayer(Layer* aLayer, Color* aOutCheckerboardingColor)
> +NeedToDrawCheckerboardingForLayer(LayerManagerComposite* aLayerManager,
> +    Layer* aLayer, Color* aOutCheckerboardingColor)
>  {
> -  return (aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE) &&
> +  return (aLayerManager->AsyncPanZoomEnabled() &&

This is fine, but you should also be able to get the LayerManager via aLayer->Manager() so you can avoid passing it in if you want.
Attachment #8734110 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #63)
> Before this lands I'd like to make sure there are no significant talos
> regressions; I'm a little worried about the overhead of running the snap
> calculations even though it's likely not very significant. Please do a try
> push with talos and talos-e10s on linux (at least) to make sure. Thanks!

Here's a Talos comparison: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5eaf176c9d83&newProject=try&newRevision=85de12ecb795&framework=1&showOnlyImportant=0

All of the regressions are "low confidence" results, seemingly just noise.
Comment on attachment 8734110 [details]
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/4-5/
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/8-9/
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/8-9/
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/8-9/
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/8-9/
I submitted the patch series via autoland.
Comment on attachment 8731503 [details]
MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40657/diff/4-5/
Comment on attachment 8732462 [details]
MozReview Request: Bug 1219296 - Factor out scroll snap information into a form that's usable by the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41197/diff/5-6/
Comment on attachment 8731505 [details]
MozReview Request: Bug 1219296 - Fix an include-what-you-use error. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40661/diff/6-7/
Comment on attachment 8731506 [details]
MozReview Request: Bug 1219296 - Factor out the algorithm that computes a scroll snap destination into a reusable form. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40663/diff/6-7/
Comment on attachment 8732463 [details]
MozReview Request: Bug 1219296 - Move the layout.css.scroll-snap.proximity-threshold pref to gfxPrefs, so it can be queried on the compositor thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41199/diff/5-6/
Comment on attachment 8734110 [details]
MozReview Request: Bug 1219296 - Make ScrollMetadata::sNullMetadata a StaticAutoPtr so that ScrollMetadata can admit nsTArray members. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42091/diff/5-6/
Comment on attachment 8731504 [details]
MozReview Request: Bug 1219296 - Ship scroll snap information to the compositor. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40659/diff/9-10/
Comment on attachment 8731507 [details]
MozReview Request: Bug 1219296 - Remove StartSmoothScroll()'s argument, which is no longer used. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40665/diff/9-10/
Comment on attachment 8731508 [details]
MozReview Request: Bug 1219296 - Light refactoring to how a smooth scroll is launched inside APZC. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40667/diff/9-10/
Comment on attachment 8731509 [details]
MozReview Request: Bug 1219296 - Scroll snap directly in APZ instead of going through the main thread. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40669/diff/9-10/
(In reply to Botond Ballo [:botond] from comment #125)
> I submitted the patch series via autoland.

(Had to rebase. Submitted again.)
(In reply to Botond Ballo [:botond] from comment #119)
> Here's a Talos comparison:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=5eaf176c9d83&newProject=try&newR
> evision=85de12ecb795&framework=1&showOnlyImportant=0
> 
> All of the regressions are "low confidence" results, seemingly just noise.

Hm, there's some scary-looking regression numbers for TART and CART and some others. But yeah, they're low confidence, possibly because the tests are bimodal or because of bug 1260926. I'm concerned that they do still introduce a real regression. We can take another look once bug 1260926 is fixed.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #138)
> Hm, there's some scary-looking regression numbers for TART and CART and some
> others. But yeah, they're low confidence, possibly because the tests are
> bimodal or because of bug 1260926. I'm concerned that they do still
> introduce a real regression. We can take another look once bug 1260926 is
> fixed.

Here are the inbound results:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=95e61ede373d&newProject=mozilla-inbound&newRevision=28a2a7730361&framework=1&filter=linu&showOnlyImportant=0

The largest regression is 1.5%.
Sounds good, thanks for checking!
^ That's pretty surprising to me, I don't see anything in the code that should cause that.
Attached image Local microbenchmark results (deleted) — 
I did some local runs; attached is the plot of the results. They're a bit noisy, but there does seem to be a pattern of improvement...
Hm, interesting. If it's easy enough, could you try to identify which patch of your patchset introduced the improvement? AFAICT none of that code should be getting exercised during this microbenchmark.
Attached image Results broken down by patch in the patch series (obsolete) (deleted) — 
Here are the results of running the microbenchmark after applying each patch in the patch series in order. The values are the average scores over 20 local runs, and the error bars are the standard deviations.

Again the results are a bit noisy, but it looks like, if any patch is responsible for the improvement, it's the first one (the ScrollMetadata refactoring).
This time with the error bars actually there.
Attachment #8740521 - Attachment is obsolete: true
Thanks for looking into this. I guess we can chalk the improvement up to removing stuff from FrameMetrics in part 1, although I think the benchmark is really quite sensitive to respond to such a change.
I got profiles of local runs before and after the refactoring, with perf [1]:

http://people.mozilla.org/~bballo/bug1219296-profile-before-refactoring
http://people.mozilla.org/~bballo/bug1219296-profile-after-refactoring

Haven't looked at / analyzed them yet, just posting them for reference.

[1] https://perf.wiki.kernel.org/index.php/Main_Page
I looked at the profiles; the most frequently sampled code was the comparison between the composited test and reference images. This is fairly incidental to the performance part of the test, which aims to measure the performance of the compositing, not of the comparison, so I commented out the comparison and got new profiles.

In the new profiles, the most frequently sampled code was in the loader, suggesting that the test wasn't spending enough time in the actual operation (compositing) compared to program startup. So I tweaked the test again, increasing the repetition count from 30 to 3000.

In the resulting test runs, the patch shows about a 5% improvement over the baseline, down from 10% before the test changes. I got new before/after profiles, and uploaded them (same links as in comment 149). The profiles are now dominated by blitting, which is more expected.

BenWa and I looked at the profiles in perf, and they looked very similar; we couldn't easily pinpoint a particular function that was responsible for the 5% difference. (Part of the reason for this is that perf's report view isn't nearly as rich as Cleopatra's UI, so it's harder to spot patterns.) Our best candidate explanation is that the changes to the FrameMetrics structure altered the code locality properties of the program.

At this point, I don't think it's worth investigating further, so I'm not going to. The profiles are available if anyone feels like looking at them more (and I can get more with other tweaks to the tests if necessary).
Comment on attachment 8731503 [details]
MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats

(Applies to all patches on this bug)

Approval Request Comment
[Feature/regressing bug #]: This patchset moves the scroll snapping implementation off the main thread and into the compositor thread
[User impact if declined]: much riskier to uplift bug 1255823, which is a fix for beta topcrasher
[Describe test coverage new/current, TreeHerder]: there are automated tests that cover this code. I did a try push with this rebased to beta (everything applied pretty cleanly) at https://treeherder.mozilla.org/#/jobs?repo=try&revision=444d4e11fab6&group_state=expanded, try push is clean except for unrelated oranges
[Risks and why]: fairly low risk, the code is well understood and there have been no regressions so far, just one follow-up patch in bug 1267471 which should be uplifted too. I'll flag that for uplift as well.
[String/UUID change made/needed]: none
Attachment #8731503 - Flags: approval-mozilla-beta?
Comment on attachment 8731503 [details]
MozReview Request: Bug 1219296 - Split fields not needed for repaints out from FrameMetrics. r=kats

We need this fix for addressing the crash spike seen in bug 1268881, I've been told this is the least risky option and this affects e10s only mode, Beta47+
Attachment #8731503 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/833474c9330b
Followup to fix stale code comments. r=me and DONTBUILD
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: