_handleTabTelemetryEnd can be too expensive

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Tabbed Browser
P1
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: Ehsan, Assigned: dao)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance] [qf:p3])

MozReview Requests

()

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

Attachments

(1 attachment)

See bug 1344302 comment 1.
Whiteboard: [qf-]
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?
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?
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Flags: needinfo?(nihsanullah)
Resolution: --- → INVALID
Uh, whoops, didn't mean to close this as INVALID.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

Updated

3 months ago
Whiteboard: [qf-] → [photon] [qf-]
(Assignee)

Comment 4

3 months ago
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?
Blocks: 828097
Good question. digitarald, do you know if anybody is paying attention to that probe?
Flags: needinfo?(hkirschner)
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.
Flags: needinfo?(nihsanullah)
Flags: needinfo?(hkirschner)
Whiteboard: [photon] [qf-] → [photon] [qf:p3]

Updated

2 months ago
Whiteboard: [photon] [qf:p3] → [qf:p3]

Updated

2 months ago
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [qf:p3] → [photon-performance] [qf:p3]
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 months ago
I also checked that tart doesn't depend on this code.
Assignee: nobody → dao+bmo
Status: REOPENED → ASSIGNED
Flags: qe-verify? → qe-verify-

Updated

2 months ago
Priority: P2 → P1

Comment 9

2 months ago
mozreview-review
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!
Attachment #8858650 - Flags: review?(mconley) → review+

Comment 10

2 months ago
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 hidden (mozreview-request)

Comment 12

2 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1685702d2d6a
Remove obsolete tab animation telemetry. r=mconley

Comment 13

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1685702d2d6a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 months ago
Iteration: --- → 55.4 - May 1
You need to log in before you can comment on or make changes to this bug.