[lldb] Avoid (unlimited) GetNumChildren calls when printing values #93946

labath · 2024-05-31T10:23:31Z

For some data formatters, even getting the number of children can be an expensive operations (e.g., needing to walk a linked list to determine the number of elements). This is then wasted work when we know we will be printing only small number of them.

This patch replaces the calls to GetNumChildren (at least those on the "frame var" path) with the calls to the capped version, passing the value of max-children-count setting (plus one)

llvmbot · 2024-05-31T10:24:00Z

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

For some data formatters, even getting the number of children can be an expensive operations (e.g., needing to walk a linked list to determine the number of elements). This is then wasted work when we know we will be printing only small number of them.

This patch replaces the calls to GetNumChildren (at least those on the "frame var" path) with the calls to the capped version, passing the value of max-children-count setting (plus one)

Full diff: https://github.com/llvm/llvm-project/pull/93946.diff

4 Files Affected:

(modified) lldb/source/DataFormatters/FormatManager.cpp (+7-3)
(modified) lldb/source/DataFormatters/ValueObjectPrinter.cpp (+14-13)
(modified) lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py (+11)
(modified) lldb/test/API/functionalities/data-formatter/synthcapping/fooSynthProvider.py (+15-1)

diff --git a/lldb/source/DataFormatters/FormatManager.cpp b/lldb/source/DataFormatters/FormatManager.cpp
index d7ba5b4b70c94..84c0c7eed1431 100644
--- a/lldb/source/DataFormatters/FormatManager.cpp
+++ b/lldb/source/DataFormatters/FormatManager.cpp
@@ -9,6 +9,7 @@
 #include "lldb/DataFormatters/FormatManager.h"
 
 #include "lldb/Core/Debugger.h"
+#include "lldb/Core/ValueObject.h"
 #include "lldb/DataFormatters/FormattersHelpers.h"
 #include "lldb/DataFormatters/LanguageCategory.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
@@ -442,16 +443,19 @@ lldb::Format FormatManager::GetSingleItemFormat(lldb::Format vector_format) {
 }
 
 bool FormatManager::ShouldPrintAsOneLiner(ValueObject &valobj) {
+  TargetSP target_sp = valobj.GetTargetSP();
   // if settings say no oneline whatsoever
-  if (valobj.GetTargetSP().get() &&
-      !valobj.GetTargetSP()->GetDebugger().GetAutoOneLineSummaries())
+  if (target_sp && !target_sp->GetDebugger().GetAutoOneLineSummaries())
     return false; // then don't oneline
 
   // if this object has a summary, then ask the summary
   if (valobj.GetSummaryFormat().get() != nullptr)
     return valobj.GetSummaryFormat()->IsOneLiner();
 
-  auto num_children = valobj.GetNumChildren();
+  size_t max_num_children =
+      (target_sp ? *target_sp : Target::GetGlobalProperties())
+          .GetMaximumNumberOfChildrenToDisplay();
+  auto num_children = valobj.GetNumChildren(max_num_children);
   if (!num_children) {
     llvm::consumeError(num_children.takeError());
     return true;
diff --git a/lldb/source/DataFormatters/ValueObjectPrinter.cpp b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
index bbdc2a9981570..c2933d8574583 100644
--- a/lldb/source/DataFormatters/ValueObjectPrinter.cpp
+++ b/lldb/source/DataFormatters/ValueObjectPrinter.cpp
@@ -14,6 +14,8 @@
 #include "lldb/Target/Language.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/Stream.h"
+#include "llvm/Support/MathExtras.h"
+#include <cstdint>
 
 using namespace lldb;
 using namespace lldb_private;
@@ -628,22 +630,21 @@ ValueObjectPrinter::GetMaxNumChildrenToPrint(bool &print_dotdotdot) {
   if (m_options.m_pointer_as_array)
     return m_options.m_pointer_as_array.m_element_count;
 
-  auto num_children_or_err = synth_valobj.GetNumChildren();
+  const uint32_t max_num_children =
+      m_options.m_ignore_cap ? UINT32_MAX
+                             : GetMostSpecializedValue()
+                                   .GetTargetSP()
+                                   ->GetMaximumNumberOfChildrenToDisplay();
+  // Ask for one more child than the maximum to see if we should print "...".
+  auto num_children_or_err = synth_valobj.GetNumChildren(
+      llvm::SaturatingAdd(max_num_children, uint32_t(1)));
   if (!num_children_or_err)
     return num_children_or_err;
-  uint32_t num_children = *num_children_or_err;
-  print_dotdotdot = false;
-  if (num_children) {
-    const size_t max_num_children = GetMostSpecializedValue()
-                                        .GetTargetSP()
-                                        ->GetMaximumNumberOfChildrenToDisplay();
-
-    if (num_children > max_num_children && !m_options.m_ignore_cap) {
-      print_dotdotdot = true;
-      return max_num_children;
-    }
+  if (*num_children_or_err > max_num_children) {
+    print_dotdotdot = true;
+    return max_num_children;
   }
-  return num_children;
+  return num_children_or_err;
 }
 
 void ValueObjectPrinter::PrintChildrenPostamble(bool print_dotdotdot) {
diff --git a/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py b/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
index d53dadef836e5..9ca232abefa03 100644
--- a/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
+++ b/lldb/test/API/functionalities/data-formatter/synthcapping/TestSyntheticCapping.py
@@ -68,6 +68,11 @@ def cleanup():
                 "r = 34",
             ],
         )
+        # num_children() should be called with at most max_num_children=257
+        # (target.max-children-count + 1)
+        self.expect(
+            "script fooSynthProvider.reset_max_num_children_max()", substrs=["257"]
+        )
 
         # check that capping works
         self.runCmd("settings set target.max-children-count 2", check=False)
@@ -80,9 +85,15 @@ def cleanup():
                 "...",
             ],
         )
+        self.expect(
+            "script fooSynthProvider.reset_max_num_children_max()", substrs=["3"]
+        )
 
         self.expect("frame variable f00_1", matching=False, substrs=["r = 34"])
 
         self.runCmd("settings set target.max-children-count 256", check=False)
 
         self.expect("frame variable f00_1", matching=True, substrs=["r = 34"])
+        self.expect(
+            "script fooSynthProvider.reset_max_num_children_max()", substrs=["257"]
+        )
diff --git a/lldb/test/API/functionalities/data-formatter/synthcapping/fooSynthProvider.py b/lldb/test/API/functionalities/data-formatter/synthcapping/fooSynthProvider.py
index 3bfa3130cc01c..3778ad9790880 100644
--- a/lldb/test/API/functionalities/data-formatter/synthcapping/fooSynthProvider.py
+++ b/lldb/test/API/functionalities/data-formatter/synthcapping/fooSynthProvider.py
@@ -2,11 +2,25 @@
 
 
 class fooSynthProvider:
+
+    # For testing purposes, we'll keep track of the maximum value of
+    # max_num_children we've been called with.
+    MAX_NUM_CHILDREN_MAX = 0
+
+    @classmethod
+    def reset_max_num_children_max(cls):
+        old_value = fooSynthProvider.MAX_NUM_CHILDREN_MAX
+        fooSynthProvider.MAX_NUM_CHILDREN_MAX = 0
+        return old_value
+
     def __init__(self, valobj, dict):
         self.valobj = valobj
         self.int_type = valobj.GetType().GetBasicType(lldb.eBasicTypeInt)
 
-    def num_children(self):
+    def num_children(self, max_num_children):
+        fooSynthProvider.MAX_NUM_CHILDREN_MAX = max(
+            fooSynthProvider.MAX_NUM_CHILDREN_MAX, max_num_children
+        )
         return 3
 
     def get_child_at_index(self, index):

github-actions · 2024-05-31T10:26:24Z

✅ With the latest revision this PR passed the Python code formatter.

For some data formatters, even getting the number of children can be an expensive operations (e.g., needing to walk a linked list to determine the number of elements). This is then wasted work when we know we will be printing only small number of them. This patch replaces the calls to GetNumChildren (at least those on the "frame var" path) with the calls to the capped version, passing the value of `max-children-count` setting (plus one)

lldb/source/DataFormatters/FormatManager.cpp

jimingham · 2024-05-31T17:53:24Z

This seems reasonable. However, I note that on the page where we show how to write synthetic child providers, we say:

def num_children(self):
this call should return the number of children that you want your object to have

That's actually not true - we pass the max_children argument on to the num_children method, and in fact some of the tests do use the max parameter. But since you're making that actually useful, can you fix the docs so people will know to take advantage of this?

jimingham · 2024-05-31T18:01:53Z

BTW, because the max number of children is probably getting ignored in the wild, this has to be advisory, you can't require that:

val.GetNumChildren(max_children) <= max_children

I don't think you do that but it might be good to note the fact.

labath · 2024-06-03T08:32:40Z

This seems reasonable. However, I note that on the page where we show how to write synthetic child providers, we say:

def num_children(self): this call should return the number of children that you want your object to have

That's actually not true - we pass the max_children argument on to the num_children method, and in fact some of the tests do use the max parameter. But since you're making that actually useful, can you fix the docs so people will know to take advantage of this?

Sounds good. I've created a separate pr for that.

BTW, because the max number of children is probably getting ignored in the wild, this has to be advisory, you can't require that:

val.GetNumChildren(max_children) <= max_children

I don't think you do that but it might be good to note the fact.

Yes, I am keeping that in mind.

This avoids us needing to enumerate the children one by one (if the pretty printer supports the extension). To keep the pretty printer interface consistent, I've define the extensions in terms of the number of children returned/yielded by the `children()` function rather than the number of entries in a map, even though this means that most map pretty printers will need to multiple the number of elemnts by two, only for GALA to divide it again. I'm also passing down the `max_count` value we get from lldb, in it's useful in some pretty printers for limiting the amount of work. With llvm/llvm-project#93946, lldb will pass down reasonable values for max_count, so we might be able to remove the limiting hack on the fallback path, but I'm leaving that for a separate patch/discussion.

…ren (#20) This avoids us needing to enumerate the children one by one (if the pretty printer supports the extension). To keep the pretty printer interface consistent, I've define the extensions in terms of the number of children returned/yielded by the `children()` function rather than the number of entries in a map, even though this means that most map pretty printers will need to multiple the number of elemnts by two, only for GALA to divide it again. I'm also passing down the `max_count` value we get from lldb, in it's useful in some pretty printers for limiting the amount of work. With llvm/llvm-project#93946, lldb will pass down reasonable values for max_count, so we might be able to remove the limiting hack on the fallback path, but I'm leaving that for a separate patch/discussion.

labath requested a review from jimingham May 31, 2024 10:23

labath requested a review from JDevlieghere as a code owner May 31, 2024 10:23

llvmbot added the lldb label May 31, 2024

labath force-pushed the num_children branch from 957f0a8 to 1e25ef1 Compare May 31, 2024 11:03

JDevlieghere approved these changes May 31, 2024

View reviewed changes

lldb/source/DataFormatters/FormatManager.cpp Outdated Show resolved Hide resolved

add const

d12612e

labath merged commit 763b96c into llvm:main Jun 3, 2024
5 checks passed

labath deleted the num_children branch June 3, 2024 09:14

labath mentioned this pull request Jun 5, 2024

Add a pretty printer extension to directly return the number of children sivachandra/gala#20

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb] Avoid (unlimited) GetNumChildren calls when printing values #93946

[lldb] Avoid (unlimited) GetNumChildren calls when printing values #93946

Uh oh!

labath commented May 31, 2024

Uh oh!

llvmbot commented May 31, 2024

Uh oh!

github-actions bot commented May 31, 2024 •

edited

Loading

Uh oh!

Uh oh!

jimingham commented May 31, 2024

Uh oh!

jimingham commented May 31, 2024

Uh oh!

labath commented Jun 3, 2024

Uh oh!

Uh oh!

Uh oh!

[lldb] Avoid (unlimited) GetNumChildren calls when printing values #93946

[lldb] Avoid (unlimited) GetNumChildren calls when printing values #93946

Uh oh!

Conversation

labath commented May 31, 2024

Uh oh!

llvmbot commented May 31, 2024

Uh oh!

github-actions bot commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jimingham commented May 31, 2024

Uh oh!

jimingham commented May 31, 2024

Uh oh!

labath commented Jun 3, 2024

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 31, 2024 •

edited

Loading