1194458 - Store bytesize of allocations in performance recordings

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: 1194458-alloc-bytesize.patch

r? shu for the tree model stuff; vp for everything else.

Nick, f?ing you for checking out mostly the tests to see if those are the expected values that we should get in a normal and inverted tree.

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

Comment 2

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

Comment on attachment 8650541 [details] [diff] [review]
1194458-alloc-bytesize.patch

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

Nice integration test coverage.

::: browser/devtools/performance/test/unit/test_tree-model-allocations-02.js
@@ +37,5 @@
> +    [100, 10, 1, 33, 100, 10, 1, 33, "x (A:1:2)"]
> +  ].forEach(compareFrameInfo(thread));
> +});
> +
> +function compareFrameInfo (root, parent) {

I feel like this could be pulled out of the test_tree-model-allocations-0{1,2}.js files and shared in a head file. Then you could also take the opportunity to document its use and params for future test readers.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8650541 [details] [diff] [review]
1194458-alloc-bytesize.patch

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

r=me for frame-utils and tree-model with comments applied.

::: browser/devtools/performance/modules/logic/frame-utils.js
@@ +511,4 @@
>      data.selfCount = node.youngestFrameSamples;
>      data.totalCount = node.samples;
> +    data.selfCountPercentage = node.youngestFrameSamples / totalSamples * 100;
> +    data.totalCountPercentage = node.samples / totalSamples * 100;

So |data| has different field names for the total sample % depending on if allocation data is displayed or not? Could you unify them?

::: browser/devtools/performance/modules/logic/tree-model.js
@@ +36,5 @@
>    this.calls = [];
>    this.duration = options.endTime - options.startTime;
>    this.nodeType = "Thread";
> +  // Total bytesize of all allocations if enabled
> +  this.totalBytes = 0;

I suggest renaming this to byteSize for consistency. Also assign youngestFrameByteSize = 0 here.

@@ +109,5 @@
>      }
>  
>      const SAMPLE_STACK_SLOT = samples.schema.stack;
>      const SAMPLE_TIME_SLOT = samples.schema.time;
> +    const SAMPLE_BYTESIZE_SLOT = samples.schema.size;

I suggest setting this conditionally, depending on if samples.schema has a 'size' property.

@@ +139,5 @@
>  
>      for (let i = 0; i < samplesData.length; i++) {
>        let sample = samplesData[i];
>        let sampleTime = sample[SAMPLE_TIME_SLOT];
> +      let byteSize = sample[SAMPLE_BYTESIZE_SLOT];

I suggest hoisting byteSize out of the loop and initializing it to 0.

Then do something like the following here:

if (SAMPLE_BYTESIZE_SLOT !== undefined) {
  byteSize = sample[SAMPLE_BYTESIZE_SLOT];
}

and making all the additions below unconditional.

@@ +291,5 @@
>  
>        let node = entry.node;
>        let calls = node.calls;
>        let callSamples = 0;
> +      let totalSize = 0;

nit: 'total' is kind of misleading here. Maybe 'callByteSize'?

Comment 4

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

Attachment: 1194458-alloc-bytesize.patch

Updated, waiting for tree to open

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

Comment 5

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

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

Comment 6

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

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

Comment 7

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

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

Comment 8

[Pulsebot]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4bbecd76b761