Closed Bug 1077438 Opened 5 years ago Closed 5 years ago

Timeline actor should manage Framerate and Memory actors

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: jsantell, Assigned: vporof)

References

Details

Attachments

(1 file, 1 obsolete file)

Just like the memory actor. This syncs all of the front-ends timeline data (memory, fps, markers), and if we land this for Gecko 35, then we will seriously reduce the amount of feature detection needed to support older Geckos.
Duplicate of this bug: 1077436
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Blocks: perf-tool-v2
Need to land this in 35.
OS: Mac OS X → All
Hardware: x86 → All
Memory actor hasn't landed yet, so need to do both things here.
Summary: Timeline actor should manage Framerate Actor → Timeline actor should manage Framerate and Memory actors
(In reply to Victor Porof [:vporof][:vp] from comment #3)
> Memory actor hasn't landed yet, so need to do both things here.

What part of the memory actor are you referring to? The actual actor has existed for a long time; the allocations methods more recently, but it has definitely landed.
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> (In reply to Victor Porof [:vporof][:vp] from comment #3)
> > Memory actor hasn't landed yet, so need to do both things here.
> 
> What part of the memory actor are you referring to? The actual actor has
> existed for a long time; the allocations methods more recently, but it has
> definitely landed.

Damn it, I accidentally some words. I meant "the memory actor usage from the timeline actor" hasn't landed yet, specifically bug 1069421.
Attached patch v1 (obsolete) — Splinter Review
Attachment #8503035 - Flags: review?(jsantell)
Attached patch v2Splinter Review
Attachment #8503035 - Attachment is obsolete: true
Attachment #8503035 - Flags: review?(jsantell)
Attachment #8503036 - Flags: review?(jsantell)
Comment on attachment 8503036 [details] [diff] [review]
v2

Review of attachment 8503036 [details] [diff] [review]:
-----------------------------------------------------------------

R+, but wonder if it'd be better to have one event for all of this data, rather than 3 separate ones, like "data" that contains memory, framerate and marker data, so the front only has to listen to one event, and they'd already be synced, which would alleviate any invalid timing issues that we were discussing before. Just a nit, I'm just happy to get this in for Fx35 regardless!
Attachment #8503036 - Flags: review?(jsantell) → review+
Emitting everything in a single event is definitely more efficient, but I really don't want to deal with backwards compatibility issues anymore. Since the current "markers" event emits arrays, it's not natural at all to augment that with other data.
Blocks: 1069421
https://hg.mozilla.org/mozilla-central/rev/661453731c9a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Setting qe-verify- for this patch, since it already has automated test coverage. Victor, if you think there's something manual QA should look at here, please flip the flag.
Flags: qe-verify-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.