Closed Bug 1338891 Opened 7 years ago Closed 7 years ago

Scroll event listener added for telemetry slows down refresh driver

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Version:
36 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox52 + fixed
firefox-esr52 --- fixed
firefox53 + fixed
firefox54 --- fixed

People

(Reporter: smaug, Assigned: rhunt)

References

Details

Attachments

(2 files, 1 obsolete file)

scroll-backout-central.patch
4.49 KB, patch
smaug
: review+
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review
scroll-backout-beta.patch
4.32 KB, patch
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review 
I was very surprised to see in performance profiles that chrome JS runs whenever scrolling happens.
Telemetry probes should be implemented in such way that they affect to performance as little as possible. So, if we need the probe, implement it in C++ please.
Is this triggering accumulations on each scroll?
While moving this to C++ will help, you could also consider just accumulating the scrolls locally and Accumulate()ing the result in Telemetry only at the end of the lifetime of the document/frame/...
I'm trying to remember why we didn't Accumulate() at the end of a document/frame, but I can't think of any reason why. That should change.

And yes, if this is going to stay for a while I agree it should be faster/in C++. Not accumulating during a scroll could be a good first step.
Attached patch scroll.patch (obsolete) — — Splinter Review 
Rough patch that translates the javascript probe into an equivalent c++ one. I don't think it should live in dom/ipc, but I don't know where would be better.
Assignee: nobody → rhunt
Attachment #8836963 - Flags: review?(bugs)
Do you have numbers of how much overhead this probe is adding to a scroll? It would be good to know that for figuring out how important it is to uplift to beta/aurora where the probe is now.
Flags: needinfo?(bugs)
Looks like in the worst case scenario (scrolling super simple document like data:text/html,<body style="height: 250000px"> ) overhead is 11% of the refreshdriver tick time.

When scrolling some taskcluster log for example (plain text only in the page) overhead in refresh driver tick is ~6%

More complicated pages make ProcessPendingUpdates take way more time and there the overhead is small.
Flags: needinfo?(bugs)
Comment on attachment 8836963 [details] [diff] [review]
scroll.patch

I was profiling this and noticed that GetScrollYOuter is accessed.
(Somehow I missed that in the js probe profile.)
Getting y flushes layout. That is really no-go for a telemetry probe. It may in worst case take, well, hundreds of milliseconds.

Can we possibly move scroll telemetry to ::FireScrollEvent?

Also, because of the layout flush, I think the js probe should be removed from branch(es?), since in some particularly way designed web pages flushing might 
affect ux really badly.

Sorry that this is rather tricky. (but I suggest using a profiling when adding a probe)
Attachment #8836963 - Flags: review?(bugs) → review-
[Tracking Requested - why for this release]:

Requesting tracking because this work is to fix a Telemetry probe that might inadvertently slow down or jank the browser when scrolling.
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Backs out the scroll tracking probes from central, I'll upload other patches for beta/aurora and flag for approval.
Attachment #8836963 - Attachment is obsolete: true
Attachment #8837843 - Flags: review?(bugs)
Comment on attachment 8837843 [details] [diff] [review]
scroll-backout-central.patch

Approval Request Comment
[Feature/Bug causing the regression]: telemetry scroll probes (bugs 1297867 + 1312881)
[User impact if declined]: scrolling can be slower or janky because of induced layout
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it removes a script that nothing depends on, that causes issues
[String changes made/needed]: none
Attachment #8837843 - Flags: approval-mozilla-aurora?
Rebased the patch above for Beta, it's the same thing.

Approval Request Comment
[Feature/Bug causing the regression]: telemetry scroll probes (bugs 1297867 + 1312881)
[User impact if declined]: scrolling can be slower or janky because of induced layout
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it removes a script that nothing depends on, that causes issues
[String changes made/needed]: none
Attachment #8837845 - Flags: approval-mozilla-beta?
Attachment #8837843 - Flags: review?(bugs) → review+
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3873032b697b
Backout telemetry scroll tracking r=smaug
https://hg.mozilla.org/mozilla-central/rev/3873032b697b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8837845 [details] [diff] [review]
scroll-backout-beta.patch

back out telemetry probes that slow down scrolling, beta52+
Attachment #8837845 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8837843 [details] [diff] [review]
scroll-backout-central.patch

backout telemetry probes that slow down scrolling, aurora53+
Attachment #8837843 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: