1345315 - _handleTabTelemetryEnd can be too expensive

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P1)

Description

[(no longer active)]

See bug 1344302 comment 1.

Comment 1

[Justin Dolske [:Dolske]]

Leaving this in the Photon backlog (for bug 1344302), but I'm not sure if this got markef qf:- because it's not really all that important? Or just isn't platform work? If it's not notable, we should maybe just close it?

Comment 2

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

I talked to Naveed about this. Apparently, qf- means "Triaged. Not a Quantum Flow bug. We don't care about this from a performance perspective for the 57 effort."

So it sounds like we should close this out - though I want to make sure we're clear on why. This view of the profile that ehsan is referring to is, I believe, illustrative:

https://perf-html.io/public/008281d4c8917aa11d4b7d59f97b2fcee471be7d/calltree/?callTreeFilters=postfixjs-Rl&range=3.5175_13.8879&thread=0

123ms over the course of a 14s profile isn't huge, but it's also not great for what should be a really cheap operation.

Was this qf-'d in triage for a reason we're not seeing here?

Comment 3

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

Uh, whoops, didn't mean to close this as INVALID.

Comment 4

[Dão Gottwald [:dao]]

I see FX_TAB_ANIM_ANY_FRAME_INTERVAL_MS for instance doesn't have alert_emails set. Is anyone actively keeping track of these probes? Looks like they're more than four years old -- what's their track record of catching regressions?

Comment 5

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

Good question. digitarald, do you know if anybody is paying attention to that probe?

Comment 6

[:Harald Kirschner :digitarald]

I looked at FX_TAB_ANIM_ANY_FRAME_INTERVAL_MS and found it only focused on the easiest path (1/2 tabs), not measuring perceived performance. Given that all animations should be on the compositor in the future, I don't see the need for having this.

Marking qf:p3 as it impacts performance and is a low hanging fruit.

Comment 7

[Dão Gottwald [:dao]]

Attachment: Bug 1345315 - Remove obsolete tab animation telemetry.

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

Comment 8

[Dão Gottwald [:dao]]

I also checked that tart doesn't depend on this code.

Comment 9

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

Comment on attachment 8858650 [details]
Bug 1345315 - Remove obsolete tab animation telemetry.

https://reviewboard.mozilla.org/r/130624/#review133940

Yeah, as the new Photon tab animation is going to be on the GPU, I don't see this being very useful anymore. Thanks dao!

Comment 10

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 48dafd25edaf -d 4e489e84adfd: rebasing 390561:48dafd25edaf "Bug 1345315 - Remove obsolete tab animation telemetry. r=mconley" (tip)
merging browser/base/content/tabbrowser.xml
merging toolkit/components/telemetry/Histograms.json
warning: conflicts while merging browser/base/content/tabbrowser.xml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8858650 [details]
Bug 1345315 - Remove obsolete tab animation telemetry.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1685702d2d6a
Remove obsolete tab animation telemetry. r=mconley

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1685702d2d6a