Closed Bug 1257369 Opened 8 years ago Closed 8 years ago

Suppress the DisplayPort when scrolling via nsSliderFrame (Scrollbar)

Categories

(Core :: Panning and Zooming, defect)

All
Unspecified
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(1 file)

There's no need to paint the DisplayPort when using sync scrolling via the ScrollBar, we should suppress it then. I'm starting to wonder if there's a more general location where we can instrument to suppress the DisplayPort.

I've seen very striking performance improvement when using this with my facebook feed. It's very jumpy when scrolling aggresively. With displayport suppression it's smooth.
This seems to work with minimal local testing. I want to test to see if I can hit the assertions before I post this for review.
Assignee: nobody → bgirard
Flags: needinfo?(bgirard)
Blocks: 1254264
Keywords: perf
Whiteboard: [gfx-noted]
Comment on attachment 8731476 [details]
MozReview Request: Bug 1257369 - Suppress the DisplayPort when scrolling via nsSliderFrame. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40629/diff/1-2/
Attachment #8731476 - Attachment description: MozReview Request: Bug 1257369 - Suppress the DisplayPort when scrolling via nsSliderFrame. → MozReview Request: Bug 1257369 - Suppress the DisplayPort when scrolling via nsSliderFrame. r=kats
Attachment #8731476 - Flags: review?(bugmail.mozilla)
I made some drive-by unification fix, I hope that's ok.
Comment on attachment 8731476 [details]
MozReview Request: Bug 1257369 - Suppress the DisplayPort when scrolling via nsSliderFrame. r=kats

https://reviewboard.mozilla.org/r/40629/#review37689

Nice, thanks!

::: layout/xul/nsSliderFrame.h:198
(Diff revision 2)
>    // ignore scrolling events as the position will be updated by APZ. If we were
>    // to process these events then the scroll position update would conflict
>    // causing the scroll position to jump.
>    bool mScrollingWithAPZ;
>  
> +  bool mActiveSuppress;

I'd prefer calling this mSuppressionActive as it's a bit clearer to me that it's reflecting the current state. Also add a comment "true if displayport suppression is active, for more performant scrollbar-dragging behaviour"

::: layout/xul/nsSliderFrame.cpp:96
(Diff revision 2)
>  }
>  
>  // stop timer
>  nsSliderFrame::~nsSliderFrame()
>  {
> +  MOZ_ASSERT(mActiveSuppress == false, "Should have un-suppress via StopDrag() first.");

Style guide says to do !mActiveSuppress instead of ==false. I don't feel strongly about it but for consistency I lean towards what the style guide says.
Attachment #8731476 - Flags: review?(bugmail.mozilla) → review+
Actually in the destructor it might be a good idea to also catch the case where the suppression is left on and explicitly call APZCCallbackHelper to disable it. The assert will catch it for debug builds but if it happens in production it would be nice to have self-correcting behaviour.
I did consider that but opted against in since I don't like shipping code I haven't been able to trigger and wouldn't be sure when it would trigger. But I had a look at SuppressDisplayport() and I think it's safe enough to do. I'll add it.
Comment on attachment 8731476 [details]
MozReview Request: Bug 1257369 - Suppress the DisplayPort when scrolling via nsSliderFrame. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40629/diff/2-3/
https://hg.mozilla.org/mozilla-central/rev/8bfe5bd9b1c7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(bgirard)
This is something we might want to uplift to 47 after baking a bit more.
Hardware: Unspecified → All
Version: unspecified → Trunk
Uplift?
Flags: needinfo?(bgirard)
Comment on attachment 8731476 [details]
MozReview Request: Bug 1257369 - Suppress the DisplayPort when scrolling via nsSliderFrame. r=kats

Approval Request Comment
[Feature/regressing bug #]: Improves APZ performance
[User impact if declined]: Jumpy scrollbar scrolling on pages like Facebook
[Describe test coverage new/current, TreeHerder]: Using the existing scrollframe test coverage
[Risks and why]: Medium risk since it changes how we scroll.
[String/UUID change made/needed]: None
Attachment #8731476 - Flags: approval-mozilla-aurora?
Comment on attachment 8731476 [details]
MozReview Request: Bug 1257369 - Suppress the DisplayPort when scrolling via nsSliderFrame. r=kats

Improves APZ performance, baked in central for ~2 weeks, seems safe to uplift to Aurora47.
Attachment #8731476 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(bgirard)
You need to log in before you can comment on or make changes to this bug.