Debug server: retrieve nested frame bindings tree lazily.

TYPE_CHANGE_OK=Proto hasn't yet been used
PiperOrigin-RevId: 202705882

6 files changed, 198 insertions, 37 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
index cc0de17909..a3df9e4291 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
@@ -35,6 +35,7 @@ message DebugRequest {
     ListFramesRequest list_frames = 104;
     StartDebuggingRequest start_debugging = 105;
     PauseThreadRequest pause_thread = 106;
+    GetChildrenRequest get_children = 107;
   }
 }
 
@@ -98,6 +99,19 @@ message PauseThreadRequest {
   int64 thread_id = 1;
 }
 
+// A request to list the children of a previously-communicated Value, such as
+// its elements (for a list or dictionary), its fields (for a struct), and so
+// forth.
+message GetChildrenRequest {
+  // The identifier of the relevant thread.
+  int64 thread_id = 1;
+
+  // The identifier of the value for which children are being requested. If the
+  // value has no children, an empty list will be returned in
+  // GetChildrenResponse.
+  int64 value_id = 2;
+}
+
 // There are two kinds of events: "responses", which correspond to a
 // DebugRequest sent by the client, and other asynchronous events that may be
 // sent by the server to notify the client of activity in the Skylark code being
@@ -118,6 +132,7 @@ message DebugEvent {
     ListFramesResponse list_frames = 104;
     StartDebuggingResponse start_debugging = 105;
     PauseThreadResponse pause_thread = 106;
+    GetChildrenResponse get_children = 107;
 
     ThreadPausedEvent thread_paused = 1001;
     ThreadContinuedEvent thread_continued = 1002;
@@ -162,6 +177,11 @@ message StartDebuggingResponse {
 message PauseThreadResponse {
 }
 
+// The response to a GetChildrenRequest.
+message GetChildrenResponse {
+  repeated Value children = 1;
+}
+
 // An event indicating that a thread was paused during execution.
 message ThreadPausedEvent {
   // The thread that was paused.
@@ -307,7 +327,12 @@ message Value {
   // themselves do not have a meaningful type with respect to our rendering.
   string type = 3;
 
-  // Any child values associated with this value, such as its elements (for a
-  // list or dictionary), its fields (for a struct), and so forth.
-  repeated Value child = 4;
+  // Will be false if the value is known to have no children. May sometimes be
+  // true if this isn't yet known, in which case GetChildrenResponse#children
+  // will be empty.
+  bool has_children = 4;
+
+  // An identifier for this value, used to request its children. The same value
+  // may be known by multiple ids. Not set for values without children.
+  int64 id = 5;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
index 648f9ab335..c0796fdc0b 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Deb
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Error;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.EvaluateResponse;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Frame;
+import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.GetChildrenResponse;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.ListFramesResponse;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.PauseThreadResponse;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.PausedThread;
@@ -101,6 +102,13 @@ final class DebugEventHelper {
         .build();
   }
 
+  static DebugEvent getChildrenResponse(long sequenceNumber, Collection<Value> children) {
+    return DebugEvent.newBuilder()
+        .setSequenceNumber(sequenceNumber)
+        .setGetChildren(GetChildrenResponse.newBuilder().addAllChildren(children))
+        .build();
+  }
+
   static DebugEvent threadPausedEvent(PausedThread thread) {
     return DebugEvent.newBuilder()
         .setThreadPaused(ThreadPausedEvent.newBuilder().setThread(thread))
@@ -129,33 +137,36 @@ final class DebugEventHelper {
         .build();
   }
 
-  static SkylarkDebuggingProtos.Frame getFrameProto(DebugFrame frame) {
+  static SkylarkDebuggingProtos.Frame getFrameProto(ThreadObjectMap objectMap, DebugFrame frame) {
     SkylarkDebuggingProtos.Frame.Builder builder =
         SkylarkDebuggingProtos.Frame.newBuilder()
             .setFunctionName(frame.functionName())
-            .addAllScope(getScopes(frame));
+            .addAllScope(getScopes(objectMap, frame));
     if (frame.location() != null) {
       builder.setLocation(getLocationProto(frame.location()));
     }
     return builder.build();
   }
 
-  private static ImmutableList<Scope> getScopes(DebugFrame frame) {
+  private static ImmutableList<Scope> getScopes(ThreadObjectMap objectMap, DebugFrame frame) {
     ImmutableMap<String, Object> localVars = frame.lexicalFrameBindings();
     if (localVars.isEmpty()) {
-      return ImmutableList.of(getScope("global", frame.globalBindings()));
+      return ImmutableList.of(getScope(objectMap, "global", frame.globalBindings()));
     }
     Map<String, Object> globalVars = new LinkedHashMap<>(frame.globalBindings());
     // remove shadowed bindings
     localVars.keySet().forEach(globalVars::remove);
 
-    return ImmutableList.of(getScope("local", localVars), getScope("global", globalVars));
+    return ImmutableList.of(
+        getScope(objectMap, "local", localVars), getScope(objectMap, "global", globalVars));
   }
 
-  private static SkylarkDebuggingProtos.Scope getScope(String name, Map<String, Object> bindings) {
+  private static SkylarkDebuggingProtos.Scope getScope(
+      ThreadObjectMap objectMap, String name, Map<String, Object> bindings) {
     SkylarkDebuggingProtos.Scope.Builder builder =
         SkylarkDebuggingProtos.Scope.newBuilder().setName(name);
-    bindings.forEach((s, o) -> builder.addBinding(DebuggerSerialization.getValueProto(s, o)));
+    bindings.forEach(
+        (s, o) -> builder.addBinding(DebuggerSerialization.getValueProto(objectMap, s, o)));
     return builder.build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebuggerSerialization.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebuggerSerialization.java
index e45a1fa294..0bc7b6fb01 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebuggerSerialization.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebuggerSerialization.java
@@ -30,14 +30,16 @@ import java.util.Map;
 /** Helper class for creating {@link SkylarkDebuggingProtos.Value} from skylark objects. */
 final class DebuggerSerialization {
 
-  static Value getValueProto(String label, Object value) {
+  static Value getValueProto(ThreadObjectMap objectMap, String label, Object value) {
     // TODO(bazel-team): prune cycles, and provide a way to limit breadth/depth of children reported
+    boolean hasChildren = hasChildren(value);
     return Value.newBuilder()
         .setLabel(label)
         // TODO(bazel-team): omit type details for non-Skylark values
         .setType(EvalUtils.getDataTypeName(value))
         .setDescription(getDescription(value))
-        .addAllChild(getChildren(value))
+        .setHasChildren(hasChildren)
+        .setId(hasChildren ? objectMap.registerValue(value) : 0)
         .build();
   }
 
@@ -52,34 +54,63 @@ final class DebuggerSerialization {
     return Value.newBuilder().setLabel("Error").setDescription(errorMessage).build();
   }
 
-  private static ImmutableList<Value> getChildren(Object value) {
+  private static boolean hasChildren(Object value) {
+    if (value instanceof ClassObject) {
+      // assuming ClassObject's have at least one child as a temporary optimization
+      // TODO(bazel-team): remove once child-listing logic is moved to SkylarkValue
+      return true;
+    }
+    if (value instanceof SkylarkNestedSet) {
+      return true;
+    }
+    if (value instanceof NestedSetView) {
+      return true;
+    }
+    if (value instanceof Map) {
+      return !((Map) value).isEmpty();
+    }
+    if (value instanceof Map.Entry) {
+      return true;
+    }
+    if (value instanceof Iterable) {
+      return ((Iterable) value).iterator().hasNext();
+    }
+    if (value.getClass().isArray()) {
+      return Array.getLength(value) > 0;
+    }
+    // fallback to assuming there are no children
+    return false;
+  }
+
+  static ImmutableList<Value> getChildren(ThreadObjectMap objectMap, Object value) {
     // TODO(bazel-team): move child-listing logic to SkylarkValue where practical
     if (value instanceof ClassObject) {
-      return getChildren((ClassObject) value);
+      return getChildren(objectMap, (ClassObject) value);
     }
     if (value instanceof SkylarkNestedSet) {
-      return getChildren((SkylarkNestedSet) value);
+      return getChildren(objectMap, (SkylarkNestedSet) value);
     }
     if (value instanceof NestedSetView) {
-      return getChildren((NestedSetView) value);
+      return getChildren(objectMap, (NestedSetView) value);
     }
     if (value instanceof Map) {
-      return getChildren(((Map) value).entrySet());
+      return getChildren(objectMap, ((Map) value).entrySet());
     }
     if (value instanceof Map.Entry) {
-      return getChildren((Map.Entry) value);
+      return getChildren(objectMap, (Map.Entry) value);
     }
     if (value instanceof Iterable) {
-      return getChildren((Iterable) value);
+      return getChildren(objectMap, (Iterable) value);
     }
     if (value.getClass().isArray()) {
-      return getArrayChildren(value);
+      return getArrayChildren(objectMap, value);
     }
     // fallback to assuming there are no children
     return ImmutableList.of();
   }
 
-  private static ImmutableList<Value> getChildren(ClassObject classObject) {
+  private static ImmutableList<Value> getChildren(
+      ThreadObjectMap objectMap, ClassObject classObject) {
     ImmutableList.Builder<Value> builder = ImmutableList.builder();
     ImmutableList<String> keys;
     try {
@@ -97,13 +128,14 @@ final class DebuggerSerialization {
                 String.format("Error retrieving value for field '%s': %s", key, e.getMessage())));
       }
       if (value != null) {
-        builder.add(getValueProto(key, value));
+        builder.add(getValueProto(objectMap, key, value));
       }
     }
     return builder.build();
   }
 
-  private static ImmutableList<Value> getChildren(SkylarkNestedSet nestedSet) {
+  private static ImmutableList<Value> getChildren(
+      ThreadObjectMap objectMap, SkylarkNestedSet nestedSet) {
     Class<?> type = nestedSet.getContentType().getType();
     return ImmutableList.<Value>builder()
         .add(
@@ -112,35 +144,38 @@ final class DebuggerSerialization {
                 .setType("Traversal order")
                 .setDescription(nestedSet.getOrder().getSkylarkName())
                 .build())
-        .addAll(getChildren(new NestedSetView<>(nestedSet.getSet(type))))
+        .addAll(getChildren(objectMap, new NestedSetView<>(nestedSet.getSet(type))))
         .build();
   }
 
-  private static ImmutableList<Value> getChildren(NestedSetView<?> nestedSet) {
+  private static ImmutableList<Value> getChildren(
+      ThreadObjectMap objectMap, NestedSetView<?> nestedSet) {
     return ImmutableList.of(
-        getValueProto("directs", nestedSet.directs()),
-        getValueProto("transitives", nestedSet.transitives()));
+        getValueProto(objectMap, "directs", nestedSet.directs()),
+        getValueProto(objectMap, "transitives", nestedSet.transitives()));
   }
 
-  private static ImmutableList<Value> getChildren(Map.Entry<?, ?> entry) {
+  private static ImmutableList<Value> getChildren(
+      ThreadObjectMap objectMap, Map.Entry<?, ?> entry) {
     return ImmutableList.of(
-        getValueProto("key", entry.getKey()), getValueProto("value", entry.getValue()));
+        getValueProto(objectMap, "key", entry.getKey()),
+        getValueProto(objectMap, "value", entry.getValue()));
   }
 
-  private static ImmutableList<Value> getChildren(Iterable<?> iterable) {
+  private static ImmutableList<Value> getChildren(ThreadObjectMap objectMap, Iterable<?> iterable) {
     ImmutableList.Builder<Value> builder = ImmutableList.builder();
     int index = 0;
     for (Object value : iterable) {
-      builder.add(getValueProto(String.format("[%d]", index++), value));
+      builder.add(getValueProto(objectMap, String.format("[%d]", index++), value));
     }
     return builder.build();
   }
 
-  private static ImmutableList<Value> getArrayChildren(Object array) {
+  private static ImmutableList<Value> getArrayChildren(ThreadObjectMap objectMap, Object array) {
     ImmutableList.Builder<Value> builder = ImmutableList.builder();
     int index = 0;
     for (int i = 0; i < Array.getLength(array); i++) {
-      builder.add(getValueProto(String.format("[%d]", index++), Array.get(array, i)));
+      builder.add(getValueProto(objectMap, String.format("[%d]", index++), Array.get(array, i)));
     }
     return builder.build();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
index 6e605895ef..d87bfdadd3 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
@@ -177,6 +177,8 @@ public final class SkylarkDebugServer implements DebugServer {
           return pauseThread(sequenceNumber, request.getPauseThread());
         case EVALUATE:
           return evaluate(sequenceNumber, request.getEvaluate());
+        case GET_CHILDREN:
+          return getChildren(sequenceNumber, request.getGetChildren());
         case PAYLOAD_NOT_SET:
           DebugEventHelper.error(sequenceNumber, "No request payload found");
       }
@@ -210,6 +212,15 @@ public final class SkylarkDebugServer implements DebugServer {
         sequenceNumber, threadHandler.evaluate(request.getThreadId(), request.getStatement()));
   }
 
+  /** Handles a {@code GetChildrenRequest} and returns its response. */
+  private SkylarkDebuggingProtos.DebugEvent getChildren(
+      long sequenceNumber, SkylarkDebuggingProtos.GetChildrenRequest request)
+      throws DebugRequestException {
+    return DebugEventHelper.getChildrenResponse(
+        sequenceNumber,
+        threadHandler.getChildrenForValue(request.getThreadId(), request.getValueId()));
+  }
+
   /** Handles a {@code ContinueExecutionRequest} and returns its response. */
   private SkylarkDebuggingProtos.DebugEvent continueExecution(
       long sequenceNumber, SkylarkDebuggingProtos.ContinueExecutionRequest request)
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
index b390b613a2..b2828f8277 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Breakpoint;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Error;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.PauseReason;
+import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Value;
 import com.google.devtools.build.lib.syntax.Debuggable;
 import com.google.devtools.build.lib.syntax.Debuggable.ReadyToPause;
 import com.google.devtools.build.lib.syntax.Environment;
@@ -50,12 +51,15 @@ final class ThreadHandler {
     /** Used to block execution of threads */
     final Semaphore semaphore;
 
+    final ThreadObjectMap objectMap;
+
     PausedThreadState(long id, String name, Debuggable debuggable, Location location) {
       this.id = id;
       this.name = name;
       this.debuggable = debuggable;
       this.location = location;
       this.semaphore = new Semaphore(0);
+      this.objectMap = new ThreadObjectMap();
     }
   }
 
@@ -230,14 +234,33 @@ final class ThreadHandler {
           .debuggable
           .listFrames(thread.location)
           .stream()
-          .map(DebugEventHelper::getFrameProto)
+          .map(frame -> DebugEventHelper.getFrameProto(thread.objectMap, frame))
           .collect(toImmutableList());
     }
   }
 
+  ImmutableList<Value> getChildrenForValue(long threadId, long valueId)
+      throws DebugRequestException {
+    ThreadObjectMap objectMap;
+    synchronized (this) {
+      PausedThreadState thread = pausedThreads.get(threadId);
+      if (thread == null) {
+        throw new DebugRequestException(
+            String.format("Thread %s is not paused or does not exist.", threadId));
+      }
+      objectMap = thread.objectMap;
+    }
+    Object value = objectMap.getValue(valueId);
+    if (value == null) {
+      throw new DebugRequestException("Couldn't retrieve children; object not found.");
+    }
+    return DebuggerSerialization.getChildren(objectMap, value);
+  }
+
   SkylarkDebuggingProtos.Value evaluate(long threadId, String statement)
       throws DebugRequestException {
     Debuggable debuggable;
+    ThreadObjectMap objectMap;
     synchronized (this) {
       PausedThreadState thread = pausedThreads.get(threadId);
       if (thread == null) {
@@ -245,13 +268,15 @@ final class ThreadHandler {
             String.format("Thread %s is not paused or does not exist.", threadId));
       }
       debuggable = thread.debuggable;
+      objectMap = thread.objectMap;
     }
-    // no need to evaluate within the synchronize block: for paused threads, debuggable is only
-    // accessed in response to a client request, and requests are handled serially
+    // no need to evaluate within the synchronize block: for paused threads, the debuggable and
+    // object map are only accessed in response to a client request, and requests are handled
+    // serially
     // TODO(bazel-team): support asynchronous replies, and use finer-grained locks
     try {
       Object result = doEvaluate(debuggable, statement);
-      return DebuggerSerialization.getValueProto("Evaluation result", result);
+      return DebuggerSerialization.getValueProto(objectMap, "Evaluation result", result);
     } catch (EvalException | InterruptedException e) {
       throw new DebugRequestException(e.getMessage());
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadObjectMap.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadObjectMap.java
new file mode 100644
index 0000000000..4e55c6105e
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadObjectMap.java
@@ -0,0 +1,54 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.skylarkdebug.server;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+import javax.annotation.Nullable;
+
+/**
+ * A map of identifier to value for all the values with children which have been communicated to the
+ * debug client for a particular, currently-paused thread.
+ *
+ * <p>Objects without children (as defined by the debugging protocol) should not be stored here.
+ *
+ * <p>Object identifiers apply only to a single thread, and aren't relevant to other threads.
+ *
+ * <p>This state is retained only while the thread is paused.
+ */
+final class ThreadObjectMap {
+  private final AtomicLong lastId = new AtomicLong(1);
+  private final Map<Long, Object> objectMap = new ConcurrentHashMap<>();
+
+  /**
+   * Assigns and returns a unique identifier for this object, and stores a reference in the
+   * identifier to object map.
+   *
+   * <p>If called multiple times for a given object, a different identifier will be returned each
+   * time. The object can be retrieved by all such identifiers.
+   */
+  long registerValue(Object value) {
+    long id = lastId.getAndIncrement();
+    objectMap.put(id, value);
+    return id;
+  }
+
+  /** Returns the object corresponding to the given identifier, if one exists. */
+  @Nullable
+  Object getValue(long identifier) {
+    return objectMap.get(identifier);
+  }
+}