Closed Bug 1174889 Opened 6 years ago Closed 6 years ago

Utility function converting samples to coordinates for stacked mountain graph

Categories

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

41 Branch
defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 1 obsolete file)

For processing optimization samples for visualizing JIT tiers over time.
Attached patch 1174889-opttiers.patch (obsolete) — Splinter Review
Attachment #8623824 - Flags: review?(vporof)
Changed so now data points with no opt sites will return an object {x: time, ys:[0,0,0]}
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dedede48bc1
Attachment #8623824 - Attachment is obsolete: true
Attachment #8623824 - Flags: review?(vporof)
Attachment #8623832 - Flags: review?(vporof)
Comment on attachment 8623832 [details] [diff] [review]
1174889-opttiers.patch

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

::: browser/devtools/performance/modules/logic/jit.js
@@ +289,5 @@
> +  let stringTable = frameNode._stringTable;
> +  let output = [];
> +  let implEnum;
> +
> +  let tierDataIterator = 0;

Nit: this is an index, not an Iterator.

@@ +304,5 @@
> +  for (let i = 0; i <= sampleTimes.length; i++) {
> +    let sampleTime = sampleTimes[i];
> +
> +    // If this sample is in the next bucket, or we're done
> +    // checking sampleTimes and on the last loop, finalize previous bucket

last iteration, not last loop.

@@ +306,5 @@
> +
> +    // If this sample is in the next bucket, or we're done
> +    // checking sampleTimes and on the last loop, finalize previous bucket
> +    if (sampleTime >= (currentBucketStartTime + bucketSize) ||
> +        i >= sampleTimes.length) {

This should also make sure there's actual samples in the current bucket. See below.

@@ +315,5 @@
> +
> +      // Map the opt site counts as a normalized percentage (0-1)
> +      // of its count in context of total samples this bucket
> +      for (let j = 0; j < IMPLEMENTATION_NAMES.length; j++) {
> +        dataPoint.ys[j] = (bucket[j] || 0) / samplesInCurrentBucket;

This has the potential to divide by 0 if the bucket size is smaller than the profiling frequency.

::: browser/devtools/performance/modules/logic/tree-model.js
@@ +229,5 @@
>          if (isLeaf) {
>            frameNode.youngestFrameSamples++;
> +          if (inflatedFrame.optimizations) {
> +            frameNode._addOptimizations(inflatedFrame.optimizations, inflatedFrame.implementation,
> +                                        sampleTime, stringTable);

This begs for an { object param }

@@ +239,5 @@
>          prevCalls = frameNode.calls;
>          isLeaf = mutableFrameKeyOptions.isLeaf = false;
>        }
>  
>        this.samples++;

We don't need the `samples` property anymore right? Could just check sampleTimes.length.
Attachment #8623832 - Flags: review?(vporof) → review+
Comment on attachment 8623832 [details] [diff] [review]
1174889-opttiers.patch

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

::: browser/devtools/performance/modules/logic/jit.js
@@ +315,5 @@
> +
> +      // Map the opt site counts as a normalized percentage (0-1)
> +      // of its count in context of total samples this bucket
> +      for (let j = 0; j < IMPLEMENTATION_NAMES.length; j++) {
> +        dataPoint.ys[j] = (bucket[j] || 0) / samplesInCurrentBucket;

Opting to just do `samplesInCurrentBucket || 1` here to prevent dividing by 0. If this aborted when checking if there were any samples, then we wouldn't have an empty bucket here, which we should to be consistent with other empty buckets (where samples exist, but no opt data)

::: browser/devtools/performance/modules/logic/tree-model.js
@@ +229,5 @@
>          if (isLeaf) {
>            frameNode.youngestFrameSamples++;
> +          if (inflatedFrame.optimizations) {
> +            frameNode._addOptimizations(inflatedFrame.optimizations, inflatedFrame.implementation,
> +                                        sampleTime, stringTable);

I agree, but nothing in this, and frame-utils does that, I think for performance reasons. I don't know

@@ +239,5 @@
>          prevCalls = frameNode.calls;
>          isLeaf = mutableFrameKeyOptions.isLeaf = false;
>        }
>  
>        this.samples++;

Once bug 1175705 is landed, I want to make this.sampleTimes only accumulate if optimizations are on, so we don't have to store a potentially large extra array for no reason
https://hg.mozilla.org/mozilla-central/rev/11e1ca981435
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.