1191508 - Allocation counts are not scoped by selection

Status: RESOLVED FIXED

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

Description

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1191508-alloc-count.patch

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.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c87ad4814005

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8644446 [details] [diff] [review]
1191508-alloc-count.patch

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.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8644446 [details] [diff] [review]
1191508-alloc-count.patch

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

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1191508-alloc-count.patch

Patch just has back end changes, and front end test changes that no longer assume the existence of a "count" property.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1191508-2-alloc-count.patch

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.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4438dc641c8e

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1191508-fix.patch

Forgot to remove tests for count for the allocations changing, failing on fx-team

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8645131 [details] [diff] [review]
1191508-fix.patch

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

Looks good to me.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a7166e6d99a5

Comment 10

[Shu-yu Guo [:shu]]

Comment on attachment 8645057 [details] [diff] [review]
1191508-2-alloc-count.patch

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.

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

try for the client side patch  https://treeherder.mozilla.org/#/jobs?repo=try&revision=dff5aedf7206

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/26f98bdef1b6

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/4438dc641c8e
https://hg.mozilla.org/mozilla-central/rev/a7166e6d99a5

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/26f98bdef1b6