Implement a new addAllGlobalsAsDebuggees method for faster chrome debugging (bug 821701); r=jimb
authorPanos Astithas <past@mozilla.com>
Fri, 04 Jan 2013 21:34:43 +0200
changeset 126695 2fce7807dc841a18adb8e17e896d45bdedce0aa7
parent 126694 a2754cd8e66bc69f4da09198a5ceb17687cdd212
child 126696 12c4b39c61f07acced16a5eae13f82091160dc06
push id2151
push userlsblakk@mozilla.com
push dateTue, 19 Feb 2013 18:06:57 +0000
treeherdermozilla-beta@4952e88741ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimb
bugs821701
milestone20.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
Implement a new addAllGlobalsAsDebuggees method for faster chrome debugging (bug 821701); r=jimb Also GC only once when going through all compartments in both addAllGlobalsAsDebuggees and removeAllDebuggees, instead of once for every debuggee added or removed.
browser/devtools/debugger/test/Makefile.in
browser/devtools/debugger/test/browser_dbg_chrome-debugging.js
js/src/jit-test/tests/debug/Debugger-debuggees-20.js
js/src/jscompartment.cpp
js/src/jscompartment.h
js/src/vm/Debugger.cpp
js/src/vm/Debugger.h
toolkit/devtools/debugger/server/dbg-script-actors.js
--- a/browser/devtools/debugger/test/Makefile.in
+++ b/browser/devtools/debugger/test/Makefile.in
@@ -82,17 +82,17 @@ MOCHITEST_BROWSER_TESTS = \
 	browser_dbg_bug786070_hide_nonenums.js \
 	browser_dbg_displayName.js \
 	browser_dbg_iframes.js \
 	browser_dbg_pause-exceptions.js \
 	browser_dbg_multiple-windows.js \
 	browser_dbg_breakpoint-new-script.js \
 	browser_dbg_bug737803_editor_actual_location.js \
 	browser_dbg_progress-listener-bug.js \
-	$(filter disabled-for-intermittent-crashes--bug-821701, browser_dbg_chrome-debugging.js) \
+	browser_dbg_chrome-debugging.js \
 	$(filter disabled-for-intermittent-failures--bug-753225, browser_dbg_createRemote.js) \
 	head.js \
 	$(NULL)
 
 ifneq ($(OS_ARCH),WINNT)
 MOCHITEST_BROWSER_TESTS += \
 	browser_dbg_bfcache.js \
 	$(NULL)
--- a/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js
+++ b/browser/devtools/debugger/test/browser_dbg_chrome-debugging.js
@@ -11,19 +11,16 @@ var gTab = null;
 var gThreadClient = null;
 var gNewGlobal = false;
 var gAttached = false;
 var gChromeScript = false;
 const DEBUGGER_TAB_URL = EXAMPLE_URL + "browser_dbg_debuggerstatement.html";
 
 function test()
 {
-  // Make sure there is enough time for findAllGlobals.
-  requestLongerTimeout(3);
-
   let transport = DebuggerServer.connectPipe();
   gClient = new DebuggerClient(transport);
   gClient.connect(function(aType, aTraits) {
     gTab = addTab(DEBUGGER_TAB_URL, function() {
       gClient.listTabs(function(aResponse) {
         let dbg = aResponse.chromeDebugger;
         ok(dbg, "Found a chrome debugging actor.");
 
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/Debugger-debuggees-20.js
@@ -0,0 +1,28 @@
+// addAllGlobalsAsDebuggees adds all the globals as debuggees.
+
+var g1 = newGlobal();           // Created before the Debugger; debuggee.
+var g2 = newGlobal();           // Created before the Debugger; not debuggee.
+
+var dbg = new Debugger;
+
+var g3 = newGlobal();           // Created after the Debugger; debuggee.
+var g4 = newGlobal();           // Created after the Debugger; not debuggee.
+
+var g1w = dbg.addDebuggee(g1);
+assertEq(dbg.addAllGlobalsAsDebuggees(), undefined);
+
+// Get Debugger.Objects viewing the globals from their own compartments;
+// this is the sort that findAllGlobals and addDebuggee return.
+var g1w = g1w.makeDebuggeeValue(g1).unwrap();
+var g2w = g1w.makeDebuggeeValue(g2).unwrap();
+var g3w = g1w.makeDebuggeeValue(g3).unwrap();
+var g4w = g1w.makeDebuggeeValue(g4).unwrap();
+var thisw = g1w.makeDebuggeeValue(this).unwrap();
+
+// Check that they're all there.
+assertEq(dbg.hasDebuggee(g1w), true);
+assertEq(dbg.hasDebuggee(g2w), true);
+assertEq(dbg.hasDebuggee(g3w), true);
+assertEq(dbg.hasDebuggee(g4w), true);
+// The debugger's global is not a debuggee.
+assertEq(dbg.hasDebuggee(thisw), false);
--- a/js/src/jscompartment.cpp
+++ b/js/src/jscompartment.cpp
@@ -879,45 +879,62 @@ JSCompartment::updateForDebugMode(FreeOp
     if (!rt->isHeapBusy())
         dmgc.scheduleGC(this);
 #endif
 }
 
 bool
 JSCompartment::addDebuggee(JSContext *cx, js::GlobalObject *global)
 {
+    AutoDebugModeGC dmgc(cx->runtime);
+    return addDebuggee(cx, global, dmgc);
+}
+
+bool
+JSCompartment::addDebuggee(JSContext *cx,
+                           js::GlobalObject *global,
+                           AutoDebugModeGC &dmgc)
+{
     bool wasEnabled = debugMode();
     if (!debuggees.put(global)) {
         js_ReportOutOfMemory(cx);
         return false;
     }
     debugModeBits |= DebugFromJS;
     if (!wasEnabled) {
-        AutoDebugModeGC dmgc(cx->runtime);
         updateForDebugMode(cx->runtime->defaultFreeOp(), dmgc);
     }
     return true;
 }
 
 void
 JSCompartment::removeDebuggee(FreeOp *fop,
                               js::GlobalObject *global,
                               js::GlobalObjectSet::Enum *debuggeesEnum)
 {
+    AutoDebugModeGC dmgc(rt);
+    return removeDebuggee(fop, global, dmgc, debuggeesEnum);
+}
+
+void
+JSCompartment::removeDebuggee(FreeOp *fop,
+                              js::GlobalObject *global,
+                              AutoDebugModeGC &dmgc,
+                              js::GlobalObjectSet::Enum *debuggeesEnum)
+{
     bool wasEnabled = debugMode();
     JS_ASSERT(debuggees.has(global));
     if (debuggeesEnum)
         debuggeesEnum->removeFront();
     else
         debuggees.remove(global);
 
     if (debuggees.empty()) {
         debugModeBits &= ~DebugFromJS;
         if (wasEnabled && !debugMode()) {
-            AutoDebugModeGC dmgc(rt);
             DebugScopes::onCompartmentLeaveDebugMode(this);
             updateForDebugMode(fop, dmgc);
         }
     }
 }
 
 void
 JSCompartment::clearBreakpointsIn(FreeOp *fop, js::Debugger *dbg, JSObject *handler)
--- a/js/src/jscompartment.h
+++ b/js/src/jscompartment.h
@@ -494,18 +494,23 @@ struct JSCompartment : private JS::shado
 
   private:
     /* This is called only when debugMode() has just toggled. */
     void updateForDebugMode(js::FreeOp *fop, js::AutoDebugModeGC &dmgc);
 
   public:
     js::GlobalObjectSet &getDebuggees() { return debuggees; }
     bool addDebuggee(JSContext *cx, js::GlobalObject *global);
+    bool addDebuggee(JSContext *cx, js::GlobalObject *global,
+                     js::AutoDebugModeGC &dmgc);
     void removeDebuggee(js::FreeOp *fop, js::GlobalObject *global,
                         js::GlobalObjectSet::Enum *debuggeesEnum = NULL);
+    void removeDebuggee(js::FreeOp *fop, js::GlobalObject *global,
+                        js::AutoDebugModeGC &dmgc,
+                        js::GlobalObjectSet::Enum *debuggeesEnum = NULL);
     bool setDebugModeFromC(JSContext *cx, bool b, js::AutoDebugModeGC &dmgc);
 
     void clearBreakpointsIn(js::FreeOp *fop, js::Debugger *dbg, JSObject *handler);
     void clearTraps(js::FreeOp *fop);
 
   private:
     void sweepBreakpoints(js::FreeOp *fop);
 
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -1877,16 +1877,36 @@ Debugger::addDebuggee(JSContext *cx, uns
     Value v = ObjectValue(*global);
     if (!dbg->wrapDebuggeeValue(cx, &v))
         return false;
     args.rval().set(v);
     return true;
 }
 
 JSBool
+Debugger::addAllGlobalsAsDebuggees(JSContext *cx, unsigned argc, Value *vp)
+{
+    THIS_DEBUGGER(cx, argc, vp, "addAllGlobalsAsDebuggees", args, dbg);
+    AutoDebugModeGC dmgc(cx->runtime);
+    for (CompartmentsIter c(cx->runtime); !c.done(); c.next()) {
+        if (c == dbg->object->compartment())
+            continue;
+        c->scheduledForDestruction = false;
+        GlobalObject *global = c->maybeGlobal();
+        if (global) {
+            Rooted<GlobalObject*> rg(cx, global);
+            dbg->addDebuggeeGlobal(cx, rg, dmgc);
+        }
+    }
+
+    args.rval().setUndefined();
+    return true;
+}
+
+JSBool
 Debugger::removeDebuggee(JSContext *cx, unsigned argc, Value *vp)
 {
     REQUIRE_ARGC("Debugger.removeDebuggee", 1);
     THIS_DEBUGGER(cx, argc, vp, "removeDebuggee", args, dbg);
     GlobalObject *global = dbg->unwrapDebuggeeArgument(cx, args[0]);
     if (!global)
         return false;
     if (dbg->debuggees.has(global))
@@ -1894,18 +1914,19 @@ Debugger::removeDebuggee(JSContext *cx, 
     args.rval().setUndefined();
     return true;
 }
 
 JSBool
 Debugger::removeAllDebuggees(JSContext *cx, unsigned argc, Value *vp)
 {
     THIS_DEBUGGER(cx, argc, vp, "removeAllDebuggees", args, dbg);
+    AutoDebugModeGC dmgc(cx->runtime);
     for (GlobalObjectSet::Enum e(dbg->debuggees); !e.empty(); e.popFront())
-        dbg->removeDebuggeeGlobal(cx->runtime->defaultFreeOp(), e.front(), NULL, &e);
+        dbg->removeDebuggeeGlobal(cx->runtime->defaultFreeOp(), e.front(), dmgc, NULL, &e);
     args.rval().setUndefined();
     return true;
 }
 
 JSBool
 Debugger::hasDebuggee(JSContext *cx, unsigned argc, Value *vp)
 {
     REQUIRE_ARGC("Debugger.hasDebuggee", 1);
@@ -2022,16 +2043,25 @@ Debugger::construct(JSContext *cx, unsig
 
     args.rval().setObject(*obj);
     return true;
 }
 
 bool
 Debugger::addDebuggeeGlobal(JSContext *cx, Handle<GlobalObject*> global)
 {
+    AutoDebugModeGC dmgc(cx->runtime);
+    return addDebuggeeGlobal(cx, global, dmgc);
+}
+
+bool
+Debugger::addDebuggeeGlobal(JSContext *cx,
+                            Handle<GlobalObject*> global,
+                            AutoDebugModeGC &dmgc)
+{
     if (debuggees.has(global))
         return true;
 
     JSCompartment *debuggeeCompartment = global->compartment();
 
     /*
      * Check for cycles. If global's compartment is reachable from this
      * Debugger object's compartment by following debuggee-to-debugger links,
@@ -2077,33 +2107,43 @@ Debugger::addDebuggeeGlobal(JSContext *c
     if (!v || !v->append(this)) {
         js_ReportOutOfMemory(cx);
     } else {
         if (!debuggees.put(global)) {
             js_ReportOutOfMemory(cx);
         } else {
             if (global->getDebuggers()->length() > 1)
                 return true;
-            if (debuggeeCompartment->addDebuggee(cx, global))
+            if (debuggeeCompartment->addDebuggee(cx, global, dmgc))
                 return true;
 
             /* Maintain consistency on error. */
             debuggees.remove(global);
         }
         JS_ASSERT(v->back() == this);
         v->popBack();
     }
     return false;
 }
 
 void
 Debugger::removeDebuggeeGlobal(FreeOp *fop, GlobalObject *global,
                                GlobalObjectSet::Enum *compartmentEnum,
                                GlobalObjectSet::Enum *debugEnum)
 {
+    AutoDebugModeGC dmgc(global->compartment()->rt);
+    return removeDebuggeeGlobal(fop, global, dmgc, compartmentEnum, debugEnum);
+}
+
+void
+Debugger::removeDebuggeeGlobal(FreeOp *fop, GlobalObject *global,
+                               AutoDebugModeGC &dmgc,
+                               GlobalObjectSet::Enum *compartmentEnum,
+                               GlobalObjectSet::Enum *debugEnum)
+{
     /*
      * Each debuggee is in two HashSets: one for its compartment and one for
      * its debugger (this). The caller might be enumerating either set; if so,
      * use HashSet::Enum::removeFront rather than HashSet::remove below, to
      * avoid invalidating the live enumerator.
      */
     JS_ASSERT(global->compartment()->getDebuggees().has(global));
     JS_ASSERT_IF(compartmentEnum, compartmentEnum->front() == global);
@@ -2146,17 +2186,17 @@ Debugger::removeDebuggeeGlobal(FreeOp *f
         debuggees.remove(global);
 
     /*
      * The debuggee needs to be removed from the compartment last, as this can
      * trigger GCs if the compartment's debug mode is being changed, and the
      * global cannot be rooted on the stack without a cx.
      */
     if (v->empty())
-        global->compartment()->removeDebuggee(fop, global, compartmentEnum);
+        global->compartment()->removeDebuggee(fop, global, dmgc, compartmentEnum);
 }
 
 /*
  * A class for parsing 'findScripts' query arguments and searching for
  * scripts that match the criteria they represent.
  */
 class Debugger::ScriptQuery {
   public:
@@ -2533,16 +2573,17 @@ JSPropertySpec Debugger::properties[] = 
     JS_PSGS("onNewGlobalObject", Debugger::getOnNewGlobalObject, Debugger::setOnNewGlobalObject, 0),
     JS_PSGS("uncaughtExceptionHook", Debugger::getUncaughtExceptionHook,
             Debugger::setUncaughtExceptionHook, 0),
     JS_PS_END
 };
 
 JSFunctionSpec Debugger::methods[] = {
     JS_FN("addDebuggee", Debugger::addDebuggee, 1, 0),
+    JS_FN("addAllGlobalsAsDebuggees", Debugger::addAllGlobalsAsDebuggees, 0, 0),
     JS_FN("removeDebuggee", Debugger::removeDebuggee, 1, 0),
     JS_FN("removeAllDebuggees", Debugger::removeAllDebuggees, 0, 0),
     JS_FN("hasDebuggee", Debugger::hasDebuggee, 1, 0),
     JS_FN("getDebuggees", Debugger::getDebuggees, 0, 0),
     JS_FN("getNewestFrame", Debugger::getNewestFrame, 0, 0),
     JS_FN("clearAllBreakpoints", Debugger::clearAllBreakpoints, 1, 0),
     JS_FN("findScripts", Debugger::findScripts, 1, 0),
     JS_FN("findAllGlobals", Debugger::findAllGlobals, 0, 0),
--- a/js/src/vm/Debugger.h
+++ b/js/src/vm/Debugger.h
@@ -220,19 +220,25 @@ class Debugger : private mozilla::Linked
 
     /* The map from debuggee Envs to Debugger.Environment instances. */
     ObjectWeakMap environments;
 
     class FrameRange;
     class ScriptQuery;
 
     bool addDebuggeeGlobal(JSContext *cx, Handle<GlobalObject*> obj);
+    bool addDebuggeeGlobal(JSContext *cx, Handle<GlobalObject*> obj,
+                           AutoDebugModeGC &dmgc);
     void removeDebuggeeGlobal(FreeOp *fop, GlobalObject *global,
                               GlobalObjectSet::Enum *compartmentEnum,
                               GlobalObjectSet::Enum *debugEnum);
+    void removeDebuggeeGlobal(FreeOp *fop, GlobalObject *global,
+                              AutoDebugModeGC &dmgc,
+                              GlobalObjectSet::Enum *compartmentEnum,
+                              GlobalObjectSet::Enum *debugEnum);
 
     /*
      * Cope with an error or exception in a debugger hook.
      *
      * If callHook is true, then call the uncaughtExceptionHook, if any. If, in
      * addition, vp is non-null, then parse the value returned by
      * uncaughtExceptionHook as a resumption value.
      *
@@ -295,16 +301,17 @@ class Debugger : private mozilla::Linked
     static JSBool setOnNewScript(JSContext *cx, unsigned argc, Value *vp);
     static JSBool getOnEnterFrame(JSContext *cx, unsigned argc, Value *vp);
     static JSBool setOnEnterFrame(JSContext *cx, unsigned argc, Value *vp);
     static JSBool getOnNewGlobalObject(JSContext *cx, unsigned argc, Value *vp);
     static JSBool setOnNewGlobalObject(JSContext *cx, unsigned argc, Value *vp);
     static JSBool getUncaughtExceptionHook(JSContext *cx, unsigned argc, Value *vp);
     static JSBool setUncaughtExceptionHook(JSContext *cx, unsigned argc, Value *vp);
     static JSBool addDebuggee(JSContext *cx, unsigned argc, Value *vp);
+    static JSBool addAllGlobalsAsDebuggees(JSContext *cx, unsigned argc, Value *vp);
     static JSBool removeDebuggee(JSContext *cx, unsigned argc, Value *vp);
     static JSBool removeAllDebuggees(JSContext *cx, unsigned argc, Value *vp);
     static JSBool hasDebuggee(JSContext *cx, unsigned argc, Value *vp);
     static JSBool getDebuggees(JSContext *cx, unsigned argc, Value *vp);
     static JSBool getNewestFrame(JSContext *cx, unsigned argc, Value *vp);
     static JSBool clearAllBreakpoints(JSContext *cx, unsigned argc, Value *vp);
     static JSBool findScripts(JSContext *cx, unsigned argc, Value *vp);
     static JSBool findAllGlobals(JSContext *cx, unsigned argc, Value *vp);
--- a/toolkit/devtools/debugger/server/dbg-script-actors.js
+++ b/toolkit/devtools/debugger/server/dbg-script-actors.js
@@ -2261,20 +2261,18 @@ update(ChromeDebuggerActor.prototype, {
    /**
    * An object that will be used by ThreadActors to tailor their behavior
    * depending on the debugging context being required (chrome or content).
    * The methods that this object provides must be bound to the ThreadActor
    * before use.
    */
   globalManager: {
     findGlobals: function CDA_findGlobals() {
-      // Fetch the list of globals from the debugger.
-      for (let g of this.dbg.findAllGlobals()) {
-        this.addDebuggee(g);
-      }
+      // Add every global known to the debugger as debuggee.
+      this.dbg.addAllGlobalsAsDebuggees();
     },
 
     /**
      * A function that the engine calls when a new global object has been
      * created.
      *
      * @param aGlobal Debugger.Object
      *        The new global object that was created.