Add "indexedVariables" to variables with lots of children.

[clayborg]

Summary

Prior to this fix if we have a really large array or collection class, we would end up always creating all of the child variables for an array or collection class. If the number of children was very high this can cause delays when expanding variables. By adding the "indexedVariables" to variables with lots of children, we can keep good performance in the variables view at all times. This patch will add the "indexedVariables" key/value pair to any "Variable" JSON dictionairies when we have an array of synthetic child provider that will create more than 100 children.

We have to be careful to not call "uint32_t SBValue::GetNumChildren()" on any lldb::SBValue that we use because it can cause a class, struct or union to complete the type in order to be able to properly tell us how many children it has and this can be expensive if you have a lot of variables. By default LLDB won't need to complete a type if we have variables that are classes, structs or unions unless the user expands the variable in the variable view. So we try to only get the GetNumChildren() when we have an array, as this is a cheap operation, or a synthetic child provider, most of which are for showing collections that typically fall into this category. We add a variable reference, which indicates that something can be expanded, when the function "bool SBValue::MightHaveChildren()" is true as this call doesn't need to complete the type in order to return true. This way if no one ever expands class variables, we don't need to complete the type.

[yinghuitan]

lldb/tools/lldb-vscode/JSONUtils.cpp

1035

It sucks that indexedVariables has be to be filled at container variable creation time to determine the children number.
A better DAP design would be VSCode asking for children count at container expansion time. Then we can always call GetNumChildren() since all children will be enumerated anyway.

1055

Do you think if we can handle the map cases? Assuming there is a formatter for unordered_map<>, if we can detect it and provide paging as well.

lldb/tools/lldb-vscode/VSCode.cpp

46 ↗

(On Diff #428537)

I assume this is your test change? Revert.

[clayborg]

Remove logging change from VSCode.cpp that was accidentally left in.

[clayborg]

lldb/tools/lldb-vscode/JSONUtils.cpp

1035

Agreed, but not much we can do there.

1055

All of the collection classes emit very similar stuff with indexed names.

std::unordered_map<std::string, std::string> u = {
        {"RED","#FF0000"},
        {"GREEN","#00FF00"},
        {"BLUE","#0000FF"}
    };

Then when we debug:

(std::unordered_map<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::hash<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<const std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >) u = size=3 {
  [0] = {
    __cc = (first = "BLUE", second = "#0000FF")
  }
  [1] = {
    __cc = (first = "GREEN", second = "#00FF00")
  }
  [2] = {
    __cc = (first = "RED", second = "#FF0000")
  }
}

So it will work for unordered_map.

[wallace]

lgtm

lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py

class TestVSCode_variables(lldbvscode_testcase.VSCodeTestCaseBase):
 
     def verify_values(self, verify_dict, actual, varref_dict=None, expression=None):
         if 'equals' in verify_dict:
             verify = verify_dict['equals']
             for key in verify:
                 verify_value = verify[key]
                 actual_value = actual[key]
                 self.assertEqual(verify_value, actual_value,
-                                '"%s" keys don\'t match (%s != %s)' % (
-                                    key, actual_value, verify_value))
+                                '"%s" keys don\'t match (%s != %s) from:\n%s' % (
+                                    key, actual_value, verify_value, actual))
         if 'startswith' in verify_dict:
             verify = verify_dict['startswith']
             for key in verify:
                 verify_value = verify[key]
                 actual_value = actual[key]
                 startswith = actual_value.startswith(verify_value)
                 self.assertTrue(startswith,
                                 ('"%s" value "%s" doesn\'t start with'
                                  ' "%s")') % (
                                     key, actual_value,
                                     verify_value))
+        if 'missing' in verify_dict:
+            missing = verify_dict['missing']
+            for key in missing:
+                self.assertTrue(key not in actual,
+                                'key "%s" is not expected in %s' % (key, actual))
         hasVariablesReference = 'variablesReference' in actual
         varRef = None
         if hasVariablesReference:
             # Remember variable references in case we want to test further
             # by using the evaluate name.
             varRef = actual["variablesReference"]
             if varRef != 0 and varref_dict is not None:
                 if expression is None:
def test_scopes_and_evaluate_expansion(self):
             len(breakpoint_ids), len(lines), "expect correct number of breakpoints"
         )
         self.continue_to_breakpoints(breakpoint_ids)
 
         # Verify locals
         locals = self.vscode.get_local_variables()
         buffer_children = make_buffer_verify_dict(0, 32)
         verify_locals = {
-            "argc": {"equals": {"type": "int", "value": "1"}},
+            "argc": {
+                "equals": {"type": "int", "value": "1"},
+                "missing": ["indexedVariables"],
+            },
             "argv": {
                 "equals": {"type": "const char **"},
                 "startswith": {"value": "0x"},
                 "hasVariablesReference": True,
+                "missing": ["indexedVariables"],
             },
             "pt": {
                 "equals": {"type": "PointType"},
                 "hasVariablesReference": True,
+                "missing": ["indexedVariables"],
                 "children": {
-                    "x": {"equals": {"type": "int", "value": "11"}},
-                    "y": {"equals": {"type": "int", "value": "22"}},
-                    "buffer": {"children": buffer_children},
-                },
+                    "x": {
+                        "equals": {"type": "int", "value": "11"},
+                        "missing": ["indexedVariables"],
+                    },
+                    "y": {
+                        "equals": {"type": "int", "value": "22"},
+                        "missing": ["indexedVariables"],
+                    },
+                    "buffer": {
+                        "children": buffer_children,
+                        "equals": {"indexedVariables": 32}
+                    },
+                },
+            "x": {
+                "equals": {"type": "int"},
+                "missing": ["indexedVariables"],
             },
-            "x": {"equals": {"type": "int"}},
         }
         self.verify_variables(verify_locals, locals)
 
         # Evaluate expandable expression twice: once permanent (from repl)
         # the other temporary (from other UI).
         expandable_expression = {
             "name": "pt",
             "response": {
                 "equals": {"type": "PointType"},
                 "startswith": {"result": "PointType @ 0x"},
+                "missing": ["indexedVariables"],
                 "hasVariablesReference": True,
             },
             "children": {
                 "x": {"equals": {"type": "int", "value": "11"}},
                 "y": {"equals": {"type": "int", "value": "22"}},
                 "buffer": {"children": buffer_children},
             },
         }
def test_scopes_and_evaluate_expansion(self):
         self.assertIn("Locals", scope_names)
         self.assertIn("Registers", scope_names)
 
         for scope in scopes:
             if scope["name"] == "Locals":
                 self.assertEquals(scope.get("presentationHint"), "locals")
             if scope["name"] == "Registers":
                 self.assertEquals(scope.get("presentationHint"), "registers")
+
+
+    @skipIfWindows
+    @skipIfRemote
+    def test_indexedVariables(self):
+        """
+        Tests that arrays and lldb.SBValue objects that have synthetic child
+        providers have "indexedVariables" key/value pairs. This helps the IDE
+        not to fetch too many children all at once.
+        """
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        source = "main.cpp"
+        breakpoint1_line = line_number(source, "// breakpoint 4")
+        lines = [breakpoint1_line]
+        # Set breakpoint in the thread function so we can step the threads
+        breakpoint_ids = self.set_source_breakpoints(source, lines)
+        self.assertEqual(
+            len(breakpoint_ids), len(lines), "expect correct number of breakpoints"
+        )
+        self.continue_to_breakpoints(breakpoint_ids)
+
+        # Verify locals
+        locals = self.vscode.get_local_variables()
+        buffer_children = make_buffer_verify_dict(0, 32)
+        verify_locals = {
+            "small_array": {"equals": {"indexedVariables": 5}},
+            "large_array": {"equals": {"indexedVariables": 200}},
+            "small_vector": {"equals": {"indexedVariables": 5}},
+            "large_vector": {"equals": {"indexedVariables": 200}},
+            "pt": {"missing": ["indexedVariables"]},
+        }
+        self.verify_variables(verify_locals, locals)

lldb/test/API/tools/lldb-vscode/variables/main.cpp

 
 #define BUFFER_SIZE 32
 struct PointType {
   int x;
   int y;
   int buffer[BUFFER_SIZE];
 };
-
+#include <vector>
 int g_global = 123;
 static int s_global = 234;
-
+int test_indexedVariables();
 int main(int argc, char const *argv[]) {
   static float s_local = 2.25;
   PointType pt = { 11,22, {0}};
   for (int i=0; i<BUFFER_SIZE; ++i)
     pt.buffer[i] = i;
   int x = s_global - g_global - pt.y; // breakpoint 1
   {
     int x = 42;
     {
       int x = 72;
       s_global = x; // breakpoint 2
     }
   }
-  return 0; // breakpoint 3
+  return test_indexedVariables(); // breakpoint 3
+}
+
+int test_indexedVariables() {
+  int small_array[5] = {1, 2, 3, 4, 5};
+  int large_array[200];
+  std::vector<int> small_vector;
+  std::vector<int> large_vector;
+  small_vector.assign(5, 0);
+  large_vector.assign(200, 0);
+  return 0; // breakpoint 4
 }

lldb/tools/lldb-vscode/JSONUtils.cpp

 //===-- JSONUtils.cpp -------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <algorithm>
 #include <iomanip>
 #include <sstream>
+#include <string.h>
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/FormatAdapters.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBBreakpointLocation.h"
llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference,
                                  bool is_name_duplicated) {
   llvm::json::Object object;
   EmplaceSafeString(object, "name",
                     CreateUniqueVariableNameForDisplay(v, is_name_duplicated));
 
   if (format_hex)
     v.SetFormat(lldb::eFormatHex);
   SetValueForKey(v, object, "value");
-  auto type_cstr = v.GetType().GetDisplayTypeName();
+  auto type_obj = v.GetType();
+  auto type_cstr = type_obj.GetDisplayTypeName();
+  // If we have a type with many many children, we would like to be able to
+  // give a hint to the IDE that the type has indexed children so that the
+  // request can be broken up in grabbing only a few children at a time. We want
+  // to be careful and only call "v.GetNumChildren()" if we have an array type
+  // or if we have a synthetic child provider. We don't want to call
+  // "v.GetNumChildren()" on all objects as class, struct and union types don't
+  // need to be completed if they are never expanded. So we want to avoid
+  // calling this to only cases where we it makes sense to keep performance high
+  // during normal debugging.
+
+  // If we have an array type, say that it is indexed and provide the number of
+  // children in case we have a huge array. If we don't do this, then we might
+  // take a while to produce all children at onces which can delay your debug
+  // session.
+  const bool is_array = type_obj.IsArrayType();
+  const bool is_synthetic = v.IsSynthetic();
+  if (is_array || is_synthetic) {
+    const auto num_children = v.GetNumChildren();
+    if (is_array) {
+      object.try_emplace("indexedVariables", num_children);
+    } else {
+      // If a type has a synthetic child provider, then the SBType of "v" won't
+      // tell us anything about what might be displayed. So we can check if the
+      // first child's name is "[0]" and then we can say it is indexed.
+      const char *first_child_name = v.GetChildAtIndex(0).GetName();
+      if (first_child_name && strcmp(first_child_name, "[0]") == 0)
+        object.try_emplace("indexedVariables", num_children);
+    }
+  }
   EmplaceSafeString(object, "type", type_cstr ? type_cstr : NO_TYPENAME);
   if (varID != INT64_MAX)
     object.try_emplace("id", varID);
   if (v.MightHaveChildren())
     object.try_emplace("variablesReference", variablesReference);
   else
     object.try_emplace("variablesReference", (int64_t)0);
   lldb::SBStream evaluateStream;