[dds/dap] Add/remove breakpoints as required instead of replacing the whole set

The DAP APIs for breakpoints send the whole set of breakpoints for a given file at once. Previously, we would just delete all breakpoints and then re-add them all, however since we added support for resolving breakpoints, this can result in all breakpoints in a file flickering to unresolved then back to resolved (as well as generally being slower).

This change splits the method that would replace all breakpoints into methods for add+remove, and then skips over any breakpoints (in setBreakpoints) that already match breakpoints we have.

Fixes Dart-Code/Dart-Code#4678

Change-Id: I489c6c295bac3ebd3a0851895a7a81e330d571e9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/433700
Reviewed-by: Ben Konyi <[email protected]>
Reviewed-by: Derek Xu <[email protected]>
Commit-Queue: Derek Xu <[email protected]>

Author: DanTup

pkg/dds/CHANGELOG.md

@@ -1,3 +1,6 @@
+# 5.0.4
+- [DAP] Breakpoints are now added/removed individually instead of all being cleared and re-added during a `setBreakpoints` request. This improves performance and can avoid breakpoints flickering between unresolved/resolved when adding new breakpoints in the same file.
+
 # 5.0.3
 - [DAP] Handle some additional errors if the VM Service is shutting down during an attempt to resume an isolate.
 - [DAP] Stack frames with dots in paths will now be parsed and have locations attached to `OutputEvents`s.

pkg/dds/lib/src/dap/adapters/dart.dart

@@ -1577,26 +1577,70 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
         : name!;
 
     // Use a completer to track when the response is sent, so any events related
-    // to these breakpoints are not sent before the client has the IDs.
+    // to new breakpoints will not be sent to the client before the response
+    // here which provides the IDs to the client.
     final completer = Completer<void>();
 
-    final clientBreakpoints = breakpoints
-        .map((bp) => ClientBreakpoint(bp, completer.future))
-        .toList();
-    await isolateManager.setBreakpoints(uri, clientBreakpoints);
-
-    sendResponse(SetBreakpointsResponseBody(
+    // Map the provided breakpoints onto either new or existing instances of
+    // [ClientBreakpoint] that we use to track the clients breakpoints
+    // internally.
+    final clientBreakpoints = breakpoints.map((bp) {
+      return
+          // First try to match an existing breakpoint so we can avoid deleting
+          // and re-creating all breakpoints if a new one is added to a file.
+          isolateManager.findExistingClientBreakpoint(uri, bp) ??
+              ClientBreakpoint(bp, completer.future);
+    }).toList();
+
+    // Any breakpoints that are not in our new set will need to be removed from
+    // the VM.
+    //
+    // Because multiple client breakpoints may resolve to the same VM breakpoint
+    // we must exclude any that still remain in one of the kept breakpoints.
+    final referencedVmBreakpoints =
+        clientBreakpoints.map((bp) => bp.forThread.values).toSet();
+    final breakpointsToRemove = isolateManager.clientBreakpointsByUri[uri]
+        ?.toSet()
+        // Remove any we're reusing.
+        .difference(clientBreakpoints.toSet())
+        // Remove any that map to VM breakpoints that are still referenced
+        // because we'll want to keep them.
+        .where((clientBreakpoint) => clientBreakpoint.forThread.values
+            .none(referencedVmBreakpoints.contains));
+
+    // Store this new set of breakpoints as the current set for this URI.
+    isolateManager.recordLatestClientBreakpoints(uri, clientBreakpoints);
+
+    // Prepare the response with the existing values before we start updating.
+    final breakpointResponse = SetBreakpointsResponseBody(
       breakpoints: clientBreakpoints
-          // Send breakpoints back as unverified and with our generated IDs so we
-          // can update them with a 'breakpoint' event when we get the
-          // 'BreakpointAdded'/'BreakpointResolved' events from the VM.
           .map((bp) => Breakpoint(
               id: bp.id,
-              verified: false,
-              message: 'Breakpoint has not yet been resolved',
-              reason: 'pending'))
+              verified: bp.verified,
+              line: bp.verified ? bp.resolvedLine : null,
+              column: bp.verified ? bp.resolvedColumn : null,
+              message: bp.verified ? null : bp.verifiedMessage,
+              reason: bp.verified ? null : bp.verifiedReason))
           .toList(),
-    ));
+    );
+
+    // Update the breakpoints for all existing threads.
+    await Future.wait(isolateManager.threads.map((thread) async {
+      // Remove the deleted breakpoints.
+      if (breakpointsToRemove != null) {
+        await Future.wait(breakpointsToRemove.map((clientBreakpoint) =>
+            isolateManager.removeBreakpoint(clientBreakpoint, thread)));
+      }
+
+      // Add the new breakpoints.
+      await Future.wait(clientBreakpoints.map((clientBreakpoint) async {
+        if (!clientBreakpoint.isKnownToVm) {
+          await isolateManager.addBreakpoint(clientBreakpoint, thread, uri);
+        }
+      }));
+    }));
+
+    sendResponse(breakpointResponse);
     completer.complete();
   }