Closed Bug 1191508 Opened 4 years ago Closed 4 years ago

Allocation counts are not scoped by selection


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

41 Branch
Not set


(firefox42 fixed)

Firefox 42
Tracking Status
firefox42 --- fixed


(Reporter: jsantell, Assigned: jsantell)


(Blocks 1 open bug, )



(3 files, 1 obsolete file)

The counts are per-frame, which can occur many times in a recording, so if the selection has one allocation at frame X, then it uses the count of allocations that appeared in the entire recording. See URL.

This is also necessary for the byte size counting.
Attached patch 1191508-alloc-count.patch (obsolete) — Splinter Review
Gets rid of counting on the actor-side -- we need to step through each individual allocation and maintain the count on the client side, because the count should change based off of selection.

We already store sample count (as leaf node and otherwise) for normal profiling trees, so this is no different, so this was super easy! And adding bytesize now will be cake.
Attachment #8644446 - Flags: review?(nfitzgerald)
Comment on attachment 8644446 [details] [diff] [review]

Review of attachment 8644446 [details] [diff] [review]:

Aside: For the future, it's really helpful for reviewers if you split your patch into multiple parts. It's a little bit more work on your end, but it ends up paying off in the long run as faster review turn around, less r-, and smaller/faster re-reviews in the case of an r-.

r=me on toolkit/devtools/*

I don't feel comfortable reviewing any of the frontend changes, as I have no idea what's going on there. Bouncing that part to Victor, but feel free to choose someone else who knows the frontend, if there is such a person.

::: toolkit/devtools/shared/memory.js
@@ -287,2 @@
>      };
> -

Nit: don't remove this line.
Attachment #8644446 - Flags: review?(vporof)
Attachment #8644446 - Flags: review?(nfitzgerald)
Attachment #8644446 - Flags: review+
Comment on attachment 8644446 [details] [diff] [review]

Review of attachment 8644446 [details] [diff] [review]:

shu's probably a better reviewer for this as these are changes to the tree model that the profiler uses. All the tests changes are just removing checking for "counts" from the allocations actor
Attachment #8644446 - Flags: review?(vporof) → review?(shu)
Patch just has back end changes, and front end test changes that no longer assume the existence of a "count" property.
Attachment #8644446 - Attachment is obsolete: true
Attachment #8644446 - Flags: review?(shu)
Attachment #8645056 - Flags: review+
And here are the tree model/view changes. Split these up so can expose bytesize on the memory module if first piece lands before uplift.
Attachment #8645057 - Flags: review?(shu)
Forgot to remove tests for count for the allocations changing, failing on fx-team
Attachment #8645131 - Flags: review?(ttromey)
Comment on attachment 8645131 [details] [diff] [review]

Review of attachment 8645131 [details] [diff] [review]:

Looks good to me.
Attachment #8645131 - Flags: review?(ttromey) → review+
Comment on attachment 8645057 [details] [diff] [review]

Review of attachment 8645057 [details] [diff] [review]:

Nice cleanup.

::: browser/devtools/performance/modules/logic/frame-utils.js
@@ +509,1 @@
>    }

We could unconditionally put the self and total counts on data. Not an expensive thing anymore like it used to be.
Attachment #8645057 - Flags: review?(shu) → review+
Keywords: leave-open
Whiteboard: [fixed-in-fx-team]
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.