fix: avoid blocking or activating the BP predictor when unnecessary

Previously, we always waited on the breakpoint predictor unless it was
explicitly disabled. This was unnecessary in two cases:

- If we were able to set an instrumentation breakpoint, this was
  redundant since we use that to ensure breakpoints get set before
  scripts run.
- We used it even if the source in question was already loaded. This is,
  while _technically_ useful if a source was present in multiple scripts
  (e.g. present in multiple webpack bundles), usually not practically
  necessary, as we do still set the breakpoint if/when that script is parsed.

Fixes microsoft/vscode#153470

Author: connor4312

CHANGELOG.md

@@ -4,7 +4,7 @@ This changelog records changes to stable releases since 1.50.2. "TBA" changes he
 
 ## Nightly (only)
 
-Nothing (yet)
+- fix: performance improvements for setting breakpoints in large projects ([vscode#153470](https://github.com/microsoft/vscode/issues/153470))
 
 ## v1.69 (June 2022)
 

src/adapter/breakpoints.ts

@@ -116,7 +116,12 @@ export class BreakpointManager {
     return this._byPath;
   }
 
-  private _sourceMapHandlerInstalled = false;
+  /**
+   * Object set once the source map handler is installed. Contains a promise
+   * that resolves to true/false based on whether a sourcemap instrumentation
+   * breakpoint (or equivalent) was able to be set.
+   */
+  private _sourceMapHandlerInstalled?: { entryBpSet: Promise<boolean> };
 
   /**
    * User-defined breakpoints by `sourceReference`.
@@ -458,34 +463,29 @@ export class BreakpointManager {
     }
   }
 
-  setSourceMapPauseDisabledForTest() {
-    // this._sourceMapPauseDisabledForTest = disabled;
-  }
-
   setPredictorDisabledForTest(disabled: boolean) {
     this._predictorDisabledForTest = disabled;
   }
 
-  private async _installSourceMapHandler(thread: Thread) {
-    this._sourceMapHandlerInstalled = true;
+  private _installSourceMapHandler(thread: Thread) {
     const perScriptSm =
       (this.launchConfig as IChromiumBaseConfiguration).perScriptSourcemaps === 'yes';
 
     if (perScriptSm) {
-      await Promise.all([
+      return Promise.all([
         this.updateEntryBreakpointMode(thread, EntryBreakpointMode.Greedy),
         thread.setScriptSourceMapHandler(false, this._scriptSourceMapHandler),
-      ]);
+      ]).then(() => true);
     } else if (this._breakpointsPredictor && !this.launchConfig.pauseForSourceMap) {
-      await thread.setScriptSourceMapHandler(false, this._scriptSourceMapHandler);
+      return thread.setScriptSourceMapHandler(false, this._scriptSourceMapHandler);
     } else {
-      await thread.setScriptSourceMapHandler(true, this._scriptSourceMapHandler);
+      return thread.setScriptSourceMapHandler(true, this._scriptSourceMapHandler);
     }
   }
 
   private async _uninstallSourceMapHandler(thread: Thread) {
     thread.setScriptSourceMapHandler(false);
-    this._sourceMapHandlerInstalled = false;
+    this._sourceMapHandlerInstalled = undefined;
   }
 
   private _setBreakpoint(b: Breakpoint, thread: Thread): void {
@@ -501,26 +501,33 @@ export class BreakpointManager {
     ids: number[],
   ): Promise<Dap.SetBreakpointsResult> {
     if (!this._sourceMapHandlerInstalled && this._thread && params.breakpoints?.length) {
-      await this._installSourceMapHandler(this._thread);
+      this._sourceMapHandlerInstalled = { entryBpSet: this._installSourceMapHandler(this._thread) };
     }
 
+    const wasEntryBpSet = await this._sourceMapHandlerInstalled?.entryBpSet;
     params.source.path = urlUtils.platformPathToPreferredCase(params.source.path);
 
     // If we see we want to set breakpoints in file by source reference ID but
     // it doesn't exist, they were probably from a previous section. The
     // references for scripts just auto-increment per session and are entirely
     // ephemeral. Remove the reference so that we fall back to a path if possible.
+    const containedSource = this._sourceContainer.source(params.source);
     if (
       params.source.sourceReference /* not (undefined or 0=on disk) */ &&
       params.source.path &&
-      !this._sourceContainer.source(params.source)
+      !containedSource
     ) {
       params.source.sourceReference = undefined;
     }
 
     // Wait until the breakpoint predictor finishes to be sure that we
-    // can place correctly in breakpoint.set().
-    if (!this._predictorDisabledForTest && this._breakpointsPredictor) {
+    // can place correctly in breakpoint.set(), if:
+    //  1) We don't have a instrumentation bp, which will be able
+    //     to pause before we hit the breakpoint
+    //  2) We already have loaded the source at least once in the runtime.
+    //     It's possible the source can be loaded again from a different script,
+    //     but we'd prefer to verify the breakpoint ASAP.
+    if (!wasEntryBpSet && this._breakpointsPredictor && !containedSource) {
       const promise = this._breakpointsPredictor.predictBreakpoints(params);
       this.addLaunchBlocker(promise);
       await promise;

src/adapter/threads.ts

@@ -1663,10 +1663,16 @@ export class Thread implements IVariableStoreLocationProvider {
     );
   }
 
+  /**
+   * Based on whether `pause` is true, sets or unsets an instrumentation
+   * breakpoint in the runtime that is hit before sources with scripts.
+   * Returns true if the breakpoint was able to be set, which is usually
+   * (always?) `false` in Node runtimes.
+   */
   public async setScriptSourceMapHandler(
     pause: boolean,
     handler?: ScriptWithSourceMapHandler,
-  ): Promise<void> {
+  ): Promise<boolean> {
     this._scriptWithSourceMapHandler = handler;
 
     const needsPause =
@@ -1681,6 +1687,8 @@ export class Thread implements IVariableStoreLocationProvider {
       this._pauseOnSourceMapBreakpointId = undefined;
       await this._cdp.Debugger.removeBreakpoint({ breakpointId });
     }
+
+    return !!this._pauseOnSourceMapBreakpointId;
   }
 
   /**

src/test/breakpoints/breakpointsTest.ts

@@ -121,7 +121,6 @@ describe('breakpoints', () => {
     itIntegrates('source map predicted', async ({ r }) => {
       // Breakpoint in source mapped script set before launch use breakpoints predictor.
       const p = await r.launchUrl('browserify/pause.html');
-      p.adapter.breakpointManager.setSourceMapPauseDisabledForTest();
       p.adapter.breakpointManager.setPredictorDisabledForTest(false);
       const source: Dap.Source = {
         path: p.workspacePath('web/browserify/module2.ts'),