Bug 1259850 - The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further, r=terrence
authorSteve Fink <sfink@mozilla.com>
Fri, 25 Mar 2016 15:12:31 -0700
changeset 338788 a122f2f80ea9598ba1ea18b588bd9e06ba415063
parent 338787 b9f523c94cf0dbbfc07e67188bb78d6c57a799f3
child 338789 d91691bc2a5dd49a1f42ab4887c61b0464edfd62
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersterrence
bugs1259850
milestone49.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1259850 - The explanations of why something is a hazard went up to just before the initial use of a variable. Extend them one further, r=terrence MozReview-Commit-ID: 9l8ftRv3yjS
js/src/devtools/rootAnalysis/analyzeRoots.js
--- a/js/src/devtools/rootAnalysis/analyzeRoots.js
+++ b/js/src/devtools/rootAnalysis/analyzeRoots.js
@@ -335,25 +335,25 @@ function findGCBeforeVariableUse(start_b
 {
     // Scan through all edges preceding an unrooted variable use, using an
     // explicit worklist, looking for a GC call. A worklist contains an
     // incoming edge together with a description of where it or one of its
     // successors GC'd (if any).
 
     var bodies_visited = new Map();
 
-    let worklist = [{body: start_body, ppoint: start_point, gcInfo: null, why: null}];
+    let worklist = [{body: start_body, ppoint: start_point, preGCLive: false, gcInfo: null, why: null}];
     while (worklist.length) {
         // Grab an entry off of the worklist, representing a point within the
         // CFG identified by <body,ppoint>. If this point has a descendant
         // later in the CFG that can GC, gcInfo will be set to the information
         // about that GC call.
 
         var entry = worklist.pop();
-        var { body, ppoint, gcInfo } = entry;
+        var { body, ppoint, gcInfo, preGCLive } = entry;
 
         // Handle the case where there are multiple ways to reach this point
         // (traversing backwards).
         var visited = bodies_visited.get(body);
         if (!visited)
             bodies_visited.set(body, visited = new Map());
         if (visited.has(ppoint)) {
             var seenEntry = visited.get(ppoint);
@@ -394,17 +394,23 @@ function findGCBeforeVariableUse(start_b
                             }
                         }
                         assert(found);
                     }
                 }
             } else if (variable.Kind == "Arg" && gcInfo) {
                 // The scope of arguments starts at the beginning of the
                 // function
-                return {gcInfo: gcInfo, why: entry};
+                return entry;
+            } else if (entry.preGCLive) {
+                // We didn't find a "good" explanation beginning of the live
+                // range, but we do know the variable was live across the GC.
+                // This can happen if the live range started when a variable is
+                // used as a retparam.
+                return entry;
             }
         }
 
         var predecessors = getPredecessors(body);
         if (!(ppoint in predecessors))
             continue;
 
         for (var edge of predecessors[ppoint]) {
@@ -420,57 +426,85 @@ function findGCBeforeVariableUse(start_b
 
             if (edge_kills) {
                 // This is a beginning of the variable's live range. If we can
                 // reach a GC call from here, then we're done -- we have a path
                 // from the beginning of the live range, through the GC call,
                 // to a use after the GC call that proves its live range
                 // extends at least that far.
                 if (gcInfo)
-                    return {gcInfo: gcInfo, why: {body: body, ppoint: source, gcInfo: gcInfo, why: entry } };
+                    return {body: body, ppoint: source, gcInfo: gcInfo, why: entry };
 
                 // Otherwise, keep searching through the graph, but truncate
                 // this particular branch of the search at this edge.
                 continue;
             }
 
+            var src_gcInfo = gcInfo;
+            var src_preGCLive = preGCLive;
             if (!gcInfo && !(source in body.suppressed) && !suppressed) {
                 var gcName = edgeCanGC(edge, body);
                 if (gcName)
-                    gcInfo = {name:gcName, body:body, ppoint:source};
+                    src_gcInfo = {name:gcName, body:body, ppoint:source};
             }
 
             if (edge_uses) {
                 // The live range starts at least this far back, so we're done
                 // for the same reason as with edge_kills. The only difference
                 // is that a GC on this edge indicates a hazard, whereas if
                 // we're killing a live range in the GC call then it's not live
                 // *across* the call.
-                if (gcInfo)
-                    return {gcInfo:gcInfo, why:entry};
+                //
+                // However, we may want to generate a longer usage chain for
+                // the variable than is minimally necessary. For example,
+                // consider:
+                //
+                //   Value v = f();
+                //   if (v.isUndefined())
+                //     return false;
+                //   gc();
+                //   return v;
+                //
+                // The call to .isUndefined() is considered to be a use and
+                // therefore indicates that v must be live at that point. But
+                // it's more helpful to the user to continue the 'why' path to
+                // include the ancestor where the value was generated. So we
+                // will only return here if edge.Kind is Assign; otherwise,
+                // we'll pass a "preGCLive" value up through the worklist to
+                // remember that the variable *is* alive before the GC and so
+                // this function should be returning a true value.
+
+                if (src_gcInfo) {
+                    src_preGCLive = true;
+                    if (edge.Kind == 'Assign')
+                        return {body:body, ppoint:source, gcInfo:src_gcInfo, why:entry};
+                }
             }
 
             if (edge.Kind == "Loop") {
                 // Additionally propagate the search into a loop body, starting
                 // with the exit point.
                 var found = false;
                 for (var xbody of functionBodies) {
                     if (sameBlockId(xbody.BlockId, edge.BlockId)) {
                         assert(!found);
                         found = true;
                         worklist.push({body:xbody, ppoint:xbody.Index[1],
-                                       gcInfo:gcInfo, why:entry});
+                                       preGCLive: src_preGCLive, gcInfo:src_gcInfo,
+                                       why:entry});
                     }
                 }
                 assert(found);
                 break;
             }
 
             // Propagate the search to the predecessors of this edge.
-            worklist.push({body:body, ppoint:source, gcInfo:gcInfo, why:entry});
+            worklist.push({body:body, ppoint:source,
+                           preGCLive: src_preGCLive, gcInfo:src_gcInfo,
+                           why:entry});
         }
     }
 
     return null;
 }
 
 function variableLiveAcrossGC(suppressed, variable)
 {
@@ -559,23 +593,31 @@ function loadPrintedLines(functionName)
                 }
             }
         }
         if (currentBody)
             currentBody.lines.push(line);
     }
 }
 
-function findLocation(body, ppoint)
+function findLocation(body, ppoint, opts={brief: false})
 {
     var location = body.PPoint[ppoint - 1].Location;
-    var text = location.CacheString + ":" + location.Line;
-    if (text.indexOf(sourceRoot) == 0)
-        return text.substring(sourceRoot.length);
-    return text;
+    var file = location.CacheString;
+
+    if (file.indexOf(sourceRoot) == 0)
+        file = file.substring(sourceRoot.length);
+
+    if (opts.brief) {
+        var m = /.*\/(.*)/.exec(file);
+        if (m)
+            file = m[1];
+    }
+
+    return file + ":" + location.Line;
 }
 
 function locationLine(text)
 {
     if (match = /:(\d+)$/.exec(text))
         return match[1];
     return 0;
 }
@@ -584,17 +626,17 @@ function printEntryTrace(functionName, e
 {
     var gcPoint = entry.gcInfo ? entry.gcInfo.ppoint : 0;
 
     if (!functionBodies[0].lines)
         loadPrintedLines(functionName);
 
     while (entry) {
         var ppoint = entry.ppoint;
-        var lineText = findLocation(entry.body, ppoint);
+        var lineText = findLocation(entry.body, ppoint, {"brief": true});
 
         var edgeText = "";
         if (entry.why && entry.why.body == entry.body) {
             // If the next point in the trace is in the same block, look for an edge between them.
             var next = entry.why.ppoint;
 
             if (!entry.body.edgeTable) {
                 var table = {};
@@ -682,17 +724,17 @@ function processBodies(functionName)
             var result = variableLiveAcrossGC(suppressed, variable.Variable);
             if (result) {
                 var lineText = findLocation(result.gcInfo.body, result.gcInfo.ppoint);
                 print("\nFunction '" + functionName + "'" +
                       " has unrooted '" + name + "'" +
                       " of type '" + typeDesc(variable.Type) + "'" +
                       " live across GC call " + result.gcInfo.name +
                       " at " + lineText);
-                printEntryTrace(functionName, result.why);
+                printEntryTrace(functionName, result);
             }
             result = unsafeVariableAddressTaken(suppressed, variable.Variable);
             if (result) {
                 var lineText = findLocation(result.body, result.ppoint);
                 print("\nFunction '" + functionName + "'" +
                       " takes unsafe address of unrooted '" + name + "'" +
                       " at " + lineText);
                 printEntryTrace(functionName, {body:result.body, ppoint:result.ppoint});