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

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
XUL Widgets
P1
normal
RESOLVED FIXED
2 months ago
29 days ago

People

(Reporter: marco, Assigned: dao)

Tracking

(Blocks: 3 bugs, {perf})

Trunk
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [ohnoreflow][qf:p1][photon-performance])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
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

Updated

2 months ago
Flags: qe-verify?
Priority: -- → P2
The sync reflow is when using the scrollTop and scrollLeft getters.

_updateScrollButtonsDisabledState may be changed to use requestAnimationFrame.

Updated

2 months ago
Component: Untriaged → Panning and Zooming
Product: Firefox → Core
Component: Panning and Zooming → XUL Widgets
Product: Core → Toolkit

Updated

2 months ago
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
See Also: → bug 1356705

Updated

2 months ago
Blocks: 1363755
No longer blocks: 1348289
Blocks: 1356153

Updated

a month ago
Flags: qe-verify? → qe-verify-
(Assignee)

Comment 2

a month ago
(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
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Keywords: perf
OS: Unspecified → All
Priority: P2 → P1
Hardware: Unspecified → All
Version: unspecified → Trunk
Perhaps this is an opportunity to use requestIdleCallback as our debouncing mechanism.
(Assignee)

Comment 4

a month ago
(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.
(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.
(Assignee)

Comment 6

a month ago
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.
(Assignee)

Comment 7

a month ago
Looks like the requestAnimationFrame callback would run before the next scroll event, so it's not useful here.
(Assignee)

Comment 8

a month ago
(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 hidden (mozreview-request)
(Assignee)

Comment 10

a month ago
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
(Assignee)

Updated

a month ago
Summary: 11.95ms uninterruptible reflow at get_scrollPosition@chrome://global/content/bindings/scrollbox.xml:201:1 → Throttle scrollbox scroll button updates while scrolling
(Assignee)

Updated

a month ago
Summary: Throttle scrollbox scroll button updates while scrolling → Throttle scrollbox scroll button disabled state updates while scrolling to avoid flushing layout
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8869605 - Flags: review?(florian)

Comment 12

a month ago
mozreview-review
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.
(Assignee)

Comment 13

a month ago
(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.
Flags: needinfo?(florian)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a month ago
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.
(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.
(Assignee)

Comment 17

a month ago
(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.
(Assignee)

Comment 18

a month ago
(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.
(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

a month ago
mozreview-review
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'.
Attachment #8869605 - Flags: review?(florian) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Flags: needinfo?(florian)

Comment 22

a month ago
Pushed by dgottwald@mozilla.com:
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

Updated

a month ago
Blocks: 1367797

Comment 23

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8e867982af7
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

a month ago
Iteration: --- → 55.6 - May 29
(Assignee)

Updated

29 days ago
Blocks: 1368208
You need to log in before you can comment on or make changes to this bug.