1358453 - Throttle scrollbox scroll button disabled state updates while scrolling to avoid flushing layout

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement, P1)

Description

[Marco Castelluccio [:marco]]

Here's the stack:

get_scrollPosition@chrome://global/content/bindings/scrollbox.xml:201:1
_updateScrollButtonsDisabledState@chrome://global/content/bindings/scrollbox.xml:623:1
onxblscroll@chrome://global/content/bindings/scrollbox.xml:799:1

Comment 1

[Florian Quèze [:florian]]

The sync reflow is when using the scrollTop and scrollLeft getters.

_updateScrollButtonsDisabledState may be changed to use requestAnimationFrame.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Marco Castelluccio [:marco] from comment #0)
> Here's the stack:
> 
> get_scrollPosition@chrome://global/content/bindings/scrollbox.xml:201:1
> _updateScrollButtonsDisabledState@chrome://global/content/bindings/scrollbox.
> xml:623:1
> onxblscroll@chrome://global/content/bindings/scrollbox.xml:799:1

Apparently the scroll event fires very frequently. We only really need to call _updateScrollButtonsDisabledState when we're done scrolling. We should probably do something like this: https://developer.mozilla.org/de/docs/Web/Events/scroll#Example

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Perhaps this is an opportunity to use requestIdleCallback as our debouncing mechanism.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #3)
> Perhaps this is an opportunity to use requestIdleCallback as our debouncing
> mechanism.

I doubt it. While constantly calling _updateScrollButtonsDisabledState during scrolling is wasteful, we do need to call it in a timely manner such that the scroll buttons' enabled/disabled don't lag behind visibly.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #4)
> I doubt it. While constantly calling _updateScrollButtonsDisabledState
> during scrolling is wasteful, we do need to call it in a timely manner such
> that the scroll buttons' enabled/disabled don't lag behind visibly.

Note that requestIdleCallback takes a timeout parameter which can be used to ensure that the callback fires within a deadline.

Comment 6

[Dão Gottwald [:dao]]

Yes, but if we set a soon enough deadline, we might as well use requestAnimationFrame like in MDN's example. requestIdleCallback would be less predictable and I don't see what benefits it would give us here.

Comment 7

[Dão Gottwald [:dao]]

Looks like the requestAnimationFrame callback would run before the next scroll event, so it's not useful here.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #7)
> Looks like the requestAnimationFrame callback would run before the next
> scroll event, so it's not useful here.

I tried two nested requestAnimationFrame calls. This kind of works, but at the end of the animation we seem to be scrolling slowly enough to have frames that don't actually scroll, so we'd still update the buttons too often.

Comment 9

[Dão Gottwald [:dao]]

Attachment: Bug 1358453 - Throttle scroll button disabled state updates while scrolling to avoid flushing layout as much as possible.

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

Comment 10

[Dão Gottwald [:dao]]

I couldn't find tests specifically for these buttons, or any other tests that would be interested in these buttons being updated synchronously while scrolling. Try run in case I missed something: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bea575df92d0a026780b39d70a483db15251bf4

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8869605 [details]
Bug 1358453 - Throttle scroll button disabled state updates while scrolling to avoid flushing layout as much as possible.

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

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8869605 [details]
Bug 1358453 - Throttle scroll button disabled state updates while scrolling to avoid flushing layout as much as possible.

https://reviewboard.mozilla.org/r/141188/#review146352

I can be convinced to r+ this as it's a nice incremental improvement over the current situation, but it's unfortunate to add a timer when we are actively trying to remove them from the front-end code.

I would be surprised if the platform code didn't know we reached a boundary while scrolling; how hard would it be to surface that information to the front-end?

How would you feel about using requestAnimationFrame with timestamps instead of the timers? I'm thinking something like:

<handler event="scroll">
  this.lastScrollEvent = now;
  this._updateScrollButtonsDisabledState(true);
  if (this.animationFrameRequested)
    return;

  let callback = () => {
    if (now - this.lastScrollEvent < 100ms) {
      requestAnimationFrame(callback);
      return;
    }
    Services.tm.dispatchToMainThread(this._updateScrollButtonsDisabledState.bind(this));
    this.animationFrameRequested = false;
  }
  requestAnimationFrame(callback);
  this.animationFrameRequested = true;
</handler>

I think this avoids the cost of the very frequently repeated setTimeout/clearTimeout calls, and the randomness of timers often firing at the worst possible time. It causes us to have the requestAnimationFrame callback be called more than once, but it's throttled to at most once per 16ms.

::: toolkit/content/widgets/scrollbox.xml:626
(Diff revision 2)
>            if (this.hasAttribute("notoverflowing")) {
>              scrolledToStart = true;
>              scrolledToEnd = true;
> +          } else if (aScrolling) {
> +            scrolledToStart = false;
> +            scrolledToEnd = false;

These 2 lines do nothing, but I understand why you added them. Maybe remove the " = false" parts of the var scrollTo{Start,End} lines, so that it doesn't look like a mistake?

::: toolkit/content/widgets/scrollbox.xml:824
(Diff revision 2)
> +        // the scroll position in case we're srolling slowly.
> +        this._delayedUpdateScrollButtonsTimer = setTimeout(() => {
> +          // Scrolling animation has finished.
> +          this._delayedUpdateScrollButtonsTimer = 0;
> +          this._updateScrollButtonsDisabledState();
> +        }, 200);

How did you pick this "200" ms constant? It's large enough to be perceiptible (but not really visible) by users. The threshold of an interaction feeling immediate is around 100ms, when something takes a bit longer (eg. 200ms), the user is less happy about the interaction but can't clearly say why.

Comment 13

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> I can be convinced to r+ this as it's a nice incremental improvement over
> the current situation, but it's unfortunate to add a timer when we are
> actively trying to remove them from the front-end code.

I don't think we have to be anal about that. Timers can still have their merit.

> I would be surprised if the platform code didn't know we reached a boundary
> while scrolling; how hard would it be to surface that information to the
> front-end?

I have no idea.

> How would you feel about using requestAnimationFrame with timestamps instead
> of the timers? I'm thinking something like:

I don't think this makes sense... see below.

> I think this avoids the cost of the very frequently repeated
> setTimeout/clearTimeout calls,

I don't understand what you mean here. setTimeout/clearTimeout calls aren't expensive as far as I know.

> and the randomness of timers often firing at
> the worst possible time.

According to mconley, we do not want to flush layout in requestAnimationFrame. If this weren't an issue, I could just use requestAnimationFrame in the setTimeout callback. But as things stand, it seems like this would be pointless if not counterproductive.

> It causes us to have the requestAnimationFrame
> callback be called more than once, but it's throttled to at most once per
> 16ms.

The setTimeout callback would get cancelled most of the time and then be called only once, which seems strictly better.

> > +        // the scroll position in case we're srolling slowly.
> > +        this._delayedUpdateScrollButtonsTimer = setTimeout(() => {
> > +          // Scrolling animation has finished.
> > +          this._delayedUpdateScrollButtonsTimer = 0;
> > +          this._updateScrollButtonsDisabledState();
> > +        }, 200);
> 
> How did you pick this "200" ms constant? It's large enough to be
> perceiptible (but not really visible) by users. The threshold of an
> interaction feeling immediate is around 100ms, when something takes a bit
> longer (eg. 200ms), the user is less happy about the interaction but can't
> clearly say why.

I tried 20ms, 50ms, 100ms, 150ms but got redundant _updateScrollButtonsDisabledState calls in all cases. 200 is what worked for me reliably.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8869605 [details]
Bug 1358453 - Throttle scroll button disabled state updates while scrolling to avoid flushing layout as much as possible.

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

Comment 15

[Dão Gottwald [:dao]]

Oh, and this:

> Services.tm.dispatchToMainThread(this._updateScrollButtonsDisabledState.
> bind(this));

would make us miss the upcoming frame, updating the buttons even later, which isn't desirable for perceived performance.

Comment 16

[Florian Quèze [:florian]]

(In reply to Dão Gottwald [::dao] from comment #13)

> > I think this avoids the cost of the very frequently repeated
> > setTimeout/clearTimeout calls,
> 
> I don't understand what you mean here. setTimeout/clearTimeout calls aren't
> expensive as far as I know.

I hope they aren't very expensive, but I've seen at least one instance where using timers (through Timer.jsm / nsITimer) and constantly resetting them like this patch does was expensive (bug 1353731 if you are curious).

> > and the randomness of timers often firing at
> > the worst possible time.
> 
> According to mconley, we do not want to flush layout in
> requestAnimationFrame.

This is the reason why my pseudo code used Services.tm.dispatchToMainThread

> The setTimeout callback would get cancelled most of the time and then be
> called only once, which seems strictly better.

setTimeout would be cancelled and reset potentially lots of time (how often is the scroll event fired? Is it hundreads of times per seconds?), whereas rAF would be retriggered at most once per frame.

(In reply to Dão Gottwald [::dao] from comment #15)
> Oh, and this:
> 
> > Services.tm.dispatchToMainThread(this._updateScrollButtonsDisabledState.
> > bind(this));
> 
> would make us miss the upcoming frame, updating the buttons even later,
> which isn't desirable for perceived performance.

If that's a concern, you can just reduce the value of the constant by 16ms.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> > > I think this avoids the cost of the very frequently repeated
> > > setTimeout/clearTimeout calls,
> > 
> > I don't understand what you mean here. setTimeout/clearTimeout calls aren't
> > expensive as far as I know.
> 
> I hope they aren't very expensive, but I've seen at least one instance where
> using timers (through Timer.jsm / nsITimer) and constantly resetting them
> like this patch does was expensive (bug 1353731 if you are curious).

I expect setTimeout and clearTimeout to optimized extensively since they're used on the web, including animation frameworks (since there was a time when we didn't have better APIs for that) and benchmarks.

> > The setTimeout callback would get cancelled most of the time and then be
> > called only once, which seems strictly better.
> 
> setTimeout would be cancelled and reset potentially lots of time (how often
> is the scroll event fired? Is it hundreads of times per seconds?), whereas
> rAF would be retriggered at most once per frame.

The scroll event fires once per frame or less if a frame doesn't move the scroll position.

> > Oh, and this:
> > 
> > > Services.tm.dispatchToMainThread(this._updateScrollButtonsDisabledState.
> > > bind(this));
> > 
> > would make us miss the upcoming frame, updating the buttons even later,
> > which isn't desirable for perceived performance.
> 
> If that's a concern, you can just reduce the value of the constant by 16ms.

The 100ms constant? That would make us call during the animation.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #17)
> > If that's a concern, you can just reduce the value of the constant by 16ms.
> 
> The 100ms constant? That would make us call during the animation.

... make us call _updateScrollButtonsDisabledState during the animation.

Comment 19

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)

> I would be surprised if the platform code didn't know we reached a boundary
> while scrolling; how hard would it be to surface that information to the
> front-end?

I used the profiler to help me locate where the scroll event is being sent from, to try to answer that question myself, and I was surprised to see that all the stacks containing _updateScrollButtonsDisabledState are starting from nsRefreshDriver::Tick.

This makes me wonder if the mdn page linked at comment 2 is still accurate. It could be that the platform has been changed since that page was written to throttle scroll events to one per frame.

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8869605 [details]
Bug 1358453 - Throttle scroll button disabled state updates while scrolling to avoid flushing layout as much as possible.

https://reviewboard.mozilla.org/r/141188/#review146414

Ok, now that we are both saying that the scroll event fires at most once per frame (which wasn't my understanding before from reading mdn), rAF doesn't make sense anymore, so r=me for the timer approach.

::: toolkit/content/widgets/scrollbox.xml:815
(Diff revision 3)
> +        }
> +
> +        // Try to detect the end of the scrolling animation to update the
> +        // scroll buttons again. To avoid false positives, this timeout needs
> +        // to be big enough to account for intermediate frames that don't move
> +        // the scroll position in case we're srolling slowly.

typo: 'srolling'.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8869605 [details]
Bug 1358453 - Throttle scroll button disabled state updates while scrolling to avoid flushing layout as much as possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141188/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8e867982af7
Throttle scroll button disabled state updates while scrolling to avoid flushing layout as much as possible. r=florian

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8e867982af7