[lldb-dap] Avoid unnecessary allocations when creating variables.

[da-viper]

This reduces unnecessary string allocations and copies when handling the variables request.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

This reduces unnecessary string allocations and copies when handling the variables request.

---

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

5 Files Affected:

• (modified) lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp (+4-6)
• (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+38-42)
• (modified) lldb/tools/lldb-dap/JSONUtils.h (+6-6)
• (modified) lldb/tools/lldb-dap/ProtocolUtils.cpp (+1-1)
• (modified) lldb/tools/lldb-dap/ProtocolUtils.h (+5-5)

diff --git a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp
index 5fa2b1ef5e20d..0177df8b754ab 100644
--- a/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/VariablesRequestHandler.cpp
@@ -26,9 +26,7 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
   const uint64_t var_ref = arguments.variablesReference;
   const uint64_t count = arguments.count;
   const uint64_t start = arguments.start;
-  bool hex = false;
-  if (arguments.format)
-    hex = arguments.format->hex;
+  const bool hex = arguments.format ? arguments.format->hex : false;
 
   std::vector<Variable> variables;
 
@@ -85,7 +83,7 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
     const int64_t end_idx = start_idx + ((count == 0) ? num_children : count);
 
     // We first find out which variable names are duplicated
-    std::map<std::string, int> variable_name_counts;
+    std::map<llvm::StringRef, int> variable_name_counts;
     for (auto i = start_idx; i < end_idx; ++i) {
       lldb::SBValue variable = top_scope->GetValueAtIndex(i);
       if (!variable.IsValid())
@@ -139,7 +137,7 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
       const bool is_permanent =
           dap.variables.IsPermanentVariableReference(var_ref);
       auto addChild = [&](lldb::SBValue child,
-                          std::optional<std::string> custom_name = {}) {
+                          std::optional<llvm::StringRef> custom_name = {}) {
         if (!child.IsValid())
           return;
         const int64_t child_var_ref =
@@ -166,7 +164,7 @@ VariablesRequestHandler::Run(const VariablesArguments &arguments) const {
     }
   }
 
-  return VariablesResponseBody{variables};
+  return VariablesResponseBody{std::move(variables)};
 }
 
 } // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 1f9719110cedb..89678c4f6bc45 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -745,66 +745,62 @@ llvm::json::Value CreateThreadStopped(DAP &dap, lldb::SBThread &thread,
   return llvm::json::Value(std::move(event));
 }
 
-const char *GetNonNullVariableName(lldb::SBValue &v) {
-  const char *name = v.GetName();
-  return name ? name : "<null>";
+llvm::StringRef GetNonNullVariableName(lldb::SBValue &v) {
+  const llvm::StringRef name = v.GetName();
+  return !name.empty() ? name : "<null>";
 }
 
 std::string CreateUniqueVariableNameForDisplay(lldb::SBValue &v,
                                                bool is_name_duplicated) {
-  lldb::SBStream name_builder;
-  name_builder.Print(GetNonNullVariableName(v));
+  std::string unique_name{};
+  llvm::raw_string_ostream name_builder(unique_name);
+  name_builder << GetNonNullVariableName(v);
   if (is_name_duplicated) {
-    lldb::SBDeclaration declaration = v.GetDeclaration();
-    const char *file_name = declaration.GetFileSpec().GetFilename();
+    const lldb::SBDeclaration declaration = v.GetDeclaration();
+    const llvm::StringRef file_name = declaration.GetFileSpec().GetFilename();
     const uint32_t line = declaration.GetLine();
 
-    if (file_name != nullptr && line > 0)
-      name_builder.Printf(" @ %s:%u", file_name, line);
-    else if (const char *location = v.GetLocation())
-      name_builder.Printf(" @ %s", location);
+    if (!file_name.empty() && line != 0 && line != LLDB_INVALID_LINE_NUMBER)
+      name_builder << llvm::formatv(" @ {}:{}", file_name, line);
+    else if (llvm::StringRef location = v.GetLocation(); !location.empty())
+      name_builder << llvm::formatv(" @ {}", location);
   }
-  return name_builder.GetData();
-}
-
-VariableDescription::VariableDescription(lldb::SBValue v,
-                                         bool auto_variable_summaries,
-                                         bool format_hex,
-                                         bool is_name_duplicated,
-                                         std::optional<std::string> custom_name)
-    : v(v) {
-  name = custom_name
-             ? *custom_name
-             : CreateUniqueVariableNameForDisplay(v, is_name_duplicated);
-
-  type_obj = v.GetType();
-  std::string raw_display_type_name =
-      llvm::StringRef(type_obj.GetDisplayTypeName()).str();
+  return unique_name;
+}
+
+VariableDescription::VariableDescription(
+    lldb::SBValue val, bool auto_variable_summaries, bool format_hex,
+    bool is_name_duplicated, std::optional<llvm::StringRef> custom_name)
+    : val(val) {
+  name = custom_name.value_or(
+      CreateUniqueVariableNameForDisplay(val, is_name_duplicated));
+
+  type_obj = val.GetType();
+  llvm::StringRef raw_display_type_name = type_obj.GetDisplayTypeName();
   display_type_name =
       !raw_display_type_name.empty() ? raw_display_type_name : NO_TYPENAME;
 
   // Only format hex/default if there is no existing special format.
-  if (v.GetFormat() == lldb::eFormatDefault ||
-      v.GetFormat() == lldb::eFormatHex) {
-    if (format_hex)
-      v.SetFormat(lldb::eFormatHex);
-    else
-      v.SetFormat(lldb::eFormatDefault);
+  if (const lldb::Format current_format = val.GetFormat();
+      current_format == lldb::eFormatDefault ||
+      current_format == lldb::eFormatHex) {
+
+    val.SetFormat(format_hex ? lldb::eFormatHex : lldb::eFormatDefault);
   }
 
   llvm::raw_string_ostream os_display_value(display_value);
 
-  if (lldb::SBError sb_error = v.GetError(); sb_error.Fail()) {
+  if (lldb::SBError sb_error = val.GetError(); sb_error.Fail()) {
     error = sb_error.GetCString();
     os_display_value << "<error: " << error << ">";
   } else {
-    value = llvm::StringRef(v.GetValue()).str();
-    summary = llvm::StringRef(v.GetSummary()).str();
+    value = val.GetValue();
+    summary = val.GetSummary();
     if (summary.empty() && auto_variable_summaries)
-      auto_summary = TryCreateAutoSummary(v);
+      auto_summary = TryCreateAutoSummary(val);
 
     std::optional<std::string> effective_summary =
-        !summary.empty() ? summary : auto_summary;
+        !summary.empty() ? summary.str() : auto_summary;
 
     if (!value.empty()) {
       os_display_value << value;
@@ -817,7 +813,7 @@ VariableDescription::VariableDescription(lldb::SBValue v,
     } else {
       if (!raw_display_type_name.empty()) {
         os_display_value << raw_display_type_name;
-        lldb::addr_t address = v.GetLoadAddress();
+        lldb::addr_t address = val.GetLoadAddress();
         if (address != LLDB_INVALID_ADDRESS)
           os_display_value << " @ " << llvm::format_hex(address, 0);
       }
@@ -825,7 +821,7 @@ VariableDescription::VariableDescription(lldb::SBValue v,
   }
 
   lldb::SBStream evaluateStream;
-  v.GetExpressionPath(evaluateStream);
+  val.GetExpressionPath(evaluateStream);
   evaluate_name = llvm::StringRef(evaluateStream.GetData()).str();
 }
 
@@ -835,13 +831,13 @@ std::string VariableDescription::GetResult(protocol::EvaluateContext context) {
   if (context != protocol::eEvaluateContextRepl)
     return display_value;
 
-  if (!v.IsValid())
+  if (!val.IsValid())
     return display_value;
 
   // Try the SBValue::GetDescription(), which may call into language runtime
   // specific formatters (see ValueObjectPrinter).
   lldb::SBStream stream;
-  v.GetDescription(stream);
+  val.GetDescription(stream);
   llvm::StringRef description = stream.GetData();
   return description.trim().str();
 }
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index 0a907599fc9ee..1e38de6f5b80d 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -316,7 +316,7 @@ llvm::json::Value CreateThreadStopped(DAP &dap, lldb::SBThread &thread,
 
 /// \return
 ///     The variable name of \a value or a default placeholder.
-const char *GetNonNullVariableName(lldb::SBValue &value);
+llvm::StringRef GetNonNullVariableName(lldb::SBValue &value);
 
 /// VSCode can't display two variables with the same name, so we need to
 /// distinguish them by using a suffix.
@@ -338,21 +338,21 @@ struct VariableDescription {
   // The variable path for this variable.
   std::string evaluate_name;
   // The output of SBValue.GetValue() if it doesn't fail. It might be empty.
-  std::string value;
+  llvm::StringRef value;
   // The summary string of this variable. It might be empty.
-  std::string summary;
+  llvm::StringRef summary;
   // The auto summary if using `enableAutoVariableSummaries`.
   std::optional<std::string> auto_summary;
   // The type of this variable.
   lldb::SBType type_obj;
   // The display type name of this variable.
-  std::string display_type_name;
+  llvm::StringRef display_type_name;
   /// The SBValue for this variable.
-  lldb::SBValue v;
+  lldb::SBValue val;
 
   VariableDescription(lldb::SBValue v, bool auto_variable_summaries,
                       bool format_hex = false, bool is_name_duplicated = false,
-                      std::optional<std::string> custom_name = {});
+                      std::optional<llvm::StringRef> custom_name = {});
 
   /// Returns a description of the value appropriate for the specified context.
   std::string GetResult(protocol::EvaluateContext context);
diff --git a/lldb/tools/lldb-dap/ProtocolUtils.cpp b/lldb/tools/lldb-dap/ProtocolUtils.cpp
index acf31b03f7af0..af583fdef52fb 100644
--- a/lldb/tools/lldb-dap/ProtocolUtils.cpp
+++ b/lldb/tools/lldb-dap/ProtocolUtils.cpp
@@ -242,7 +242,7 @@ CreateExceptionBreakpointFilter(const ExceptionBreakpoint &bp) {
 Variable CreateVariable(lldb::SBValue v, int64_t var_ref, bool format_hex,
                         bool auto_variable_summaries,
                         bool synthetic_child_debugging, bool is_name_duplicated,
-                        std::optional<std::string> custom_name) {
+                        std::optional<llvm::StringRef> custom_name) {
   VariableDescription desc(v, auto_variable_summaries, format_hex,
                            is_name_duplicated, custom_name);
   Variable var;
diff --git a/lldb/tools/lldb-dap/ProtocolUtils.h b/lldb/tools/lldb-dap/ProtocolUtils.h
index f4d576ba9f608..89839f2caad50 100644
--- a/lldb/tools/lldb-dap/ProtocolUtils.h
+++ b/lldb/tools/lldb-dap/ProtocolUtils.h
@@ -143,11 +143,11 @@ std::string ConvertDebugInfoSizeToString(uint64_t debug_size);
 ///
 /// \return
 ///     A Variable representing the given value.
-protocol::Variable CreateVariable(lldb::SBValue v, int64_t var_ref,
-                                  bool format_hex, bool auto_variable_summaries,
-                                  bool synthetic_child_debugging,
-                                  bool is_name_duplicated,
-                                  std::optional<std::string> custom_name = {});
+protocol::Variable
+CreateVariable(lldb::SBValue v, int64_t var_ref, bool format_hex,
+               bool auto_variable_summaries, bool synthetic_child_debugging,
+               bool is_name_duplicated,
+               std::optional<llvm::StringRef> custom_name = {});
 
 } // namespace lldb_dap
 

[DrSergei]

Is this patch inspired by some performance issues? It looks like that using llvm::StringRef in VariableDescription might be less safe. Maybe we can add getters for fields which directly obtained from SBValue. It is just some thoughts, feel free to ignore it. Overall, LGTM

[DrSergei]

nit: llvm::StringRef effective_summary = auto_summary ? *auto_summary : summary and check that !effective_summary.empty() below

[da-viper]

Is this patch inspired by some performance issues? It looks like that using llvm::StringRef in VariableDescription might be less

char * gotten from any SBAPI functions are owned by the lldb process. and the VariableDescription still has a reference to SBValue