Closed Bug 1194458 Opened 5 years ago Closed 5 years ago

Store bytesize of allocations in performance recordings

Categories

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

41 Branch
defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch 1194458-alloc-bytesize.patch (obsolete) — Splinter Review
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
Attachment #8650541 - Flags: review?(vporof)
Attachment #8650541 - Flags: review?(shu)
Attachment #8650541 - Flags: feedback?(nfitzgerald)
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.
Attachment #8650541 - Flags: feedback?(nfitzgerald) → feedback+
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'?
Attachment #8650541 - Flags: review?(shu) → review+
Attachment #8650541 - Flags: review?(vporof) → review+
https://hg.mozilla.org/mozilla-central/rev/4bbecd76b761
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.