Bug 1617666 - Use a separate Debugger to improve performance of eval. r=jlast

The SpiderMonkey Debugger API maintains a flag per-debuggee that tracks whether
the SpiderMonkey should notify the debugger when new frames are entered, and
whether the JIT scripts associated with the debuggee realm have been compiled
with debug instrumentation. To avoid constantly needing to clear and regenerate
JIT scripts, the flag is permanently enabled once "onEnterFrame" has been used
with a given Debugger instance, and when the flag is enabled, there is a
runtime performance cost.

This runtime cost is what is causing the performance regression here, so to
ensure that the flag is cleared at the end of a given instant-eval, we only
set "onEnterFrame" on a new temporary Debugger instance, instead of setting
it on th existing persistent Debugger instance.

Differential Revision: https://phabricator.services.mozilla.com/D64912

--HG--
extra : moz-landing-system : lando

devtools/server/actors/webconsole/eval-with-debugger.js

@@ -5,6 +5,7 @@
 
 "use strict";
 
+const Debugger = require("Debugger");
 const DevToolsUtils = require("devtools/shared/DevToolsUtils");
 
 loader.lazyRequireGetter(
@@ -162,7 +163,7 @@ exports.evalWithDebugger = function(string, options = {}, webConsole) {
   );
 
   if (options.eager) {
-    allowSideEffects(dbg, sideEffectData);
+    allowSideEffects(sideEffectData);
   }
 
   // Attempt to initialize any declarations found in the evaluated string
@@ -268,17 +269,17 @@ function preventSideEffects(dbg) {
     throw new Error("Debugger has hook installed");
   }
 
-  const data = {
-    executedScripts: new Set(),
-    debuggees: dbg.getDebuggees(),
-
-    handler: {
-      hit: () => null,
-    },
-  };
+  // Note: It is critical for debuggee performance that we implement all of
+  // this debuggee tracking logic with a separate Debugger instance.
+  // Bug 1617666 arises otherwise if we set an onEnterFrame hook on the
+  // existing debugger object and then later clear it.
+  const newDbg = new Debugger();
+  for (const debuggee of dbg.getDebuggees()) {
+    newDbg.addDebuggee(debuggee.unsafeDereference());
+  }
 
   // TODO: re-enable addAllGlobalsAsDebuggees(bug #1610532)
-  // dbg.addAllGlobalsAsDebuggees();
+  // newDbg.addAllGlobalsAsDebuggees();
 
   const timeoutDuration = 100;
   const endTime = Date.now() + timeoutDuration;
@@ -290,7 +291,12 @@ function preventSideEffects(dbg) {
     return ++count % 100 === 0 && Date.now() > endTime;
   }
 
-  dbg.onEnterFrame = frame => {
+  const executedScripts = new Set();
+  const handler = {
+    hit: () => null,
+  };
+
+  newDbg.onEnterFrame = frame => {
     if (shouldCancel()) {
       return null;
     }
@@ -303,19 +309,22 @@ function preventSideEffects(dbg) {
 
     const script = frame.script;
 
-    if (data.executedScripts.has(script)) {
+    if (executedScripts.has(script)) {
       return undefined;
     }
-    data.executedScripts.add(script);
+    executedScripts.add(script);
 
     const offsets = script.getEffectfulOffsets();
     for (const offset of offsets) {
-      script.setBreakpoint(offset, data.handler);
+      script.setBreakpoint(offset, handler);
     }
 
     return undefined;
   };
 
+  // The debugger only calls onNativeCall handlers on the debugger that is
+  // explicitly calling eval, so we need to add this hook on "dbg" even though
+  // the rest of our hooks work via "newDbg".
   dbg.onNativeCall = (callee, reason) => {
     // Getters are never considered effectful, and setters are always effectful.
     // Natives called normally are handled with a whitelist.
@@ -330,22 +339,15 @@ function preventSideEffects(dbg) {
     return null;
   };
 
-  return data;
+  return {
+    dbg,
+    newDbg,
+  };
 }
 
-function allowSideEffects(dbg, data) {
-  for (const script of data.executedScripts) {
-    script.clearBreakpoint(data.handler);
-  }
-
-  for (const global of dbg.getDebuggees()) {
-    if (!data.debuggees.includes(global)) {
-      dbg.removeDebuggee(global);
-    }
-  }
-
-  dbg.onEnterFrame = undefined;
-  dbg.onNativeCall = undefined;
+function allowSideEffects(data) {
+  data.dbg.onNativeCall = undefined;
+  data.newDbg.removeAllDebuggees();
 }
 
 // Native functions which are considered to be side effect free.