Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
  • 3 commits
  • 3 files changed
  • 7 comments
  • 1 contributor
Oct 20, 2012
Nickolay Ponomarev Make sample.frames always non-null.
In filtered samples, null frames (sample.frames == null) and
null samples (sample == null) seem to be treated the same,
except for the cases where the null frames are not handled at all.

This changes chargeNonJSToCallers to null out the sample instead
of frames, so that sample.frames is always non-null.

This fixes an issue with missing sample.frames null-check
in filterByCallstackPrefix() by making it non-necessary.
The missing null-check caused an exception when turning on the
"Javascript only" filter while focusing (via
FocusedCallstackPrefixSampleFilter)
on a portion of a stack that didn't have any JS in it.

I couldn't find explanation of why the null frames were
needed in the first place in the commits that initially
added this code:
bgirard@055a8dc
bgirard@7162f03
5e5103e
Nickolay Ponomarev Add diagnostics for add-on code.
Subsequent samples with at least one frame belonging to an add-on will
get marked as such.

addon.png taken from http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/mozapps/extensions/extensionGeneric-16.png
62ea978
Nickolay Ponomarev Don't diagnose non-consecuitive samples.
Before this change filtering samples on a name that matches a diagnostic
would display this diagnostic even though the samples are far apart.
05df8b8
BIN  images/diagnostic/addon.png
8  js/diagnosticBar.js
@@ -11,7 +11,7 @@ DiagnosticBar.prototype = {
11 11
   setDetailsListener: function(callback) {
12 12
     this._detailsListener = callback;
13 13
   },
14  
-  _addDiagnosticItem: function(x, width, imageFile, title, details, onclickDetails) {
  14
+  _addDiagnosticItem: function(x, width, imageFile, imageURL, title, details, onclickDetails) {
15 15
     var self = this;
16 16
     x = x * 100;
17 17
     width = width * 100;
@@ -26,7 +26,8 @@ DiagnosticBar.prototype = {
26 26
 
27 27
     var diagnostic = document.createElement("a");
28 28
 
29  
-    var backgroundImageStr = "url('images/diagnostic/"+imageFile+"')";
  29
+    var backgroundImageStr = imageURL ? "url('" + imageURL + "')" :
  30
+                                        "url('images/diagnostic/"+imageFile+"')";
30 31
     diagnostic.style.position = "absolute";
31 32
     diagnostic.style.backgroundImage = backgroundImageStr;
32 33
     diagnostic.style.backgroundRepeat = "no-repeat";
@@ -62,7 +63,8 @@ DiagnosticBar.prototype = {
62 63
     this._container.innerHTML = "";
63 64
 
64 65
     var addedAnyDiagnosticItem = diagnosticItems.map(function addOneItem(item) {
65  
-      return self._addDiagnosticItem(item.x, item.width, item.imageFile, item.title, item.details, item.onclickDetails);
  66
+      return self._addDiagnosticItem(item.x, item.width, item.imageFile, item.imageURL,
  67
+                                     item.title, item.details, item.onclickDetails);
66 68
     }).some(function (didAdd) { return didAdd; });
67 69
 
68 70
     if (!addedAnyDiagnosticItem) {
53  js/parserWorker.js
@@ -764,7 +764,6 @@ function convertToCallTree(samples, isReverse) {
764 764
   function areSamplesMultiroot(samples) {
765 765
     var previousRoot;
766 766
     for (var i = 0; i < samples.length; ++i) {
767  
-      if (!samples[i].frames) continue;
768 767
       if (!previousRoot) {
769 768
         previousRoot = samples[i].frames[0];
770 769
         continue;
@@ -782,7 +781,6 @@ function convertToCallTree(samples, isReverse) {
782 781
     return new TreeNode("(empty)", null, 0);
783 782
   var firstRoot = null;
784 783
   for (var i = 0; i < samples.length; ++i) {
785  
-    if (!samples[i].frames) continue;
786 784
     firstRoot = samples[i].frames[0];
787 785
     break;
788 786
   }
@@ -793,9 +791,6 @@ function convertToCallTree(samples, isReverse) {
793 791
   var treeRoot = new TreeNode((isReverse || multiRoot) ? "(total)" : firstRoot, null, 0);
794 792
   for (var i = 0; i < samples.length; ++i) {
795 793
     var sample = samples[i];
796  
-    if (!sample.frames) {
797  
-      continue;
798  
-    }
799 794
     var callstack = sample.frames.slice(0);
800 795
     callstack.shift();
801 796
     if (isReverse)
@@ -896,11 +891,11 @@ function chargeNonJSToCallers(samples, symbols, functions, useFunctions) {
896 891
       }
897 892
     }
898 893
     if (!newFrames.length) {
899  
-      newFrames = null;
  894
+      samples[i] = null;
900 895
     } else {
901 896
       newFrames.splice(0, 0, "(total)");
  897
+      samples[i].frames = newFrames;
902 898
     }
903  
-    samples[i].frames = newFrames;
904 899
   }
905 900
   return samples;
906 901
 }
@@ -1076,7 +1071,7 @@ function updateFilters(requestID, profileID, filters) {
1076 1071
   gProfiles[profileID].filterSettings = filters;
1077 1072
   gProfiles[profileID].filteredSamples = samples;
1078 1073
   sendFinishedInChunks(requestID, samples, 40000,
1079  
-                       function (sample) { return (sample && sample.frames) ? sample.frames.length : 1; });
  1074
+                       function (sample) { return sample ? sample.frames.length : 1; });
1080 1075
 }
1081 1076
 
1082 1077
 function updateViewOptions(requestID, profileID, options) {
@@ -1114,7 +1109,7 @@ function calculateHistogramData(requestID, profileID) {
1114 1109
   for (var i = 0; i < data.length; ++i) {
1115 1110
     if (!data[i])
1116 1111
       continue;
1117  
-    var value = data[i].frames ? data[i].frames.length : 0;
  1112
+    var value = data[i].frames.length;
1118 1113
     if (maxHeight < value)
1119 1114
       maxHeight = value;
1120 1115
   }
@@ -1128,7 +1123,7 @@ function calculateHistogramData(requestID, profileID) {
1128 1123
   var frameStart = {};
1129 1124
   for (var i = 0; i < data.length; i++) {
1130 1125
     var step = data[i];
1131  
-    if (!step || !step.frames) {
  1126
+    if (!step) {
1132 1127
       // Add a gap for the sample that was filtered out.
1133 1128
       nextX += 1 / samplesPerStep;
1134 1129
       continue;
@@ -1501,6 +1496,30 @@ function calculateDiagnosticItems(requestID, profileID, meta) {
1501 1496
 
1502 1497
   var diagnosticItems = [];
1503 1498
 
  1499
+  // add diagnostics specific to this profile.
  1500
+  var tmpDiagnosticList = diagnosticList.concat(
  1501
+    Object.keys(profile.resources)
  1502
+      .map(function(resourceKey) {
  1503
+        var resource = profile.resources[resourceKey];
  1504
+        return (resource.type != "addon") ? null : {
  1505
+          image: "addon.png",
  1506
+          imageURL: resource.icon,
  1507
+          title: "Add-on: " + resource.name,
  1508
+          check: function(frames, symbols, meta) {
  1509
+            for (var i = 0; i < frames.length; i++) {
  1510
+              if (symbols[frames[i]].libraryName == resourceKey) {
  1511
+                return true;
  1512
+              }
  1513
+            }
  1514
+            return false;
  1515
+          }
  1516
+        };
  1517
+      })
  1518
+      .filter(function(diagnostic) {
  1519
+        return diagnostic != null;
  1520
+      })
  1521
+  );
  1522
+
1504 1523
   function finishPendingDiagnostic(endX) {
1505 1524
     if (!pendingDiagnosticInfo)
1506 1525
       return;
@@ -1510,6 +1529,7 @@ function calculateDiagnosticItems(requestID, profileID, meta) {
1510 1529
       x: pendingDiagnosticInfo.x / widthSum,
1511 1530
       width: (endX - pendingDiagnosticInfo.x) / widthSum,
1512 1531
       imageFile: pendingDiagnosticInfo.diagnostic.image,
  1532
+      imageURL: pendingDiagnosticInfo.diagnostic.imageURL,
1513 1533
       title: pendingDiagnosticInfo.diagnostic.title,
1514 1534
       details: pendingDiagnosticInfo.details,
1515 1535
       onclickDetails: pendingDiagnosticInfo.onclickDetails
@@ -1532,14 +1552,13 @@ function calculateDiagnosticItems(requestID, profileID, meta) {
1532 1552
 */
1533 1553
 
1534 1554
   data.forEach(function diagnoseStep(step, x) {
1535  
-    if (!step)
1536  
-      return;
  1555
+    if (step) {
  1556
+      var frames = step.frames;
1537 1557
 
1538  
-    var frames = step.frames;
1539  
-
1540  
-    var diagnostic = firstMatch(diagnosticList, function (diagnostic) {
1541  
-      return diagnostic.check(frames, symbols, meta, step);
1542  
-    });
  1558
+      var diagnostic = firstMatch(tmpDiagnosticList, function (diagnostic) {
  1559
+        return diagnostic.check(frames, symbols, meta, step);
  1560
+      });
  1561
+    }
1543 1562
 
1544 1563
     if (!diagnostic) {
1545 1564
       finishPendingDiagnostic(x);

Showing you all comments on commits in this comparison.

Benoit Girard

Ohh cool, I've been meaning to fix this bug

Benoit Girard

We could use the add-on's homepage as a link. Clicking on the diagnostic would bring you to the addon page.

Benoit Girard

The goal of the diagnostics bar is to show real or potential problems. If I read this correctly this will unconditionally show any add-ons execution. I'm afraid this will make the bar too noisy. If you're only interested in showing them during startup you could check for relevant startup symbols on the stack.

Nickolay Ponomarev
Owner

@bgirard: extension code running is always a potential problem :)

I thought relying on the diagnostic width filter here https://github.com/bgirard/cleopatra/blame/master/js/diagnosticBar.js#LID18 would limit the noisiness to an acceptable level, since as far as I understand, the diagnostic will display only for >=1% of consecutive samples containing a frame associated with a particular extension. What am I missing?

Nickolay Ponomarev
Owner

@bgirard: that's an option, but I don't think it's worth the code to support it, since visiting the homepage is unlikely to help finding details about the extension's behavior in the profile.

Benoit Girard

The problem is that 1% of the profile may or may not be a significant amount of time. For the same reason we don't attribute things like layout, painting and other easy to identify event. Otherwise the bar will be full of markers and drown out the important issues such as sync reflows.

Nickolay Ponomarev
Owner

@bgirard: got it, thanks for the clarification. Should I submit a new pull request without this commit to make it easier for you to land the other two commits?

I think it's useful to identify known blocks of code, possibly including painting and layout (to make it easier for the cleopatra user to get the big picture and drill down into interesting and/or unexpected blocks). But I see how it's out of scope for the diagnostics bar, even though it could use the same visualization mechanism. Do you think this would be useful as a separate bar?

Something went wrong with that request. Please try again.