Reduce extraneous temp strings in debugserver, free objects when they're not longer needed

[jasonmolenda]

Summary

Looking at debugserver's memory use a bit, I noticed that the SendPacket method which takes a std::string& argument, is often called with a std::string object that the caller calls c_str() on, so a temporary copy of the string is created. This patch removes those c_str() method calls.

We also have a common pattern where we have/create a JSONGenerator::ObjectSP, dump the string representation of the object into a ostringstream, and then binary escape the ostringstream. We end up with the original ObjectSP dictionary, the ostringstream print of it, and the escaped std::string copy of it. Then we pass that to SendPacket which may create a compressed std::string of it. This patch frees those objects as soon as they are no longer needed, so our peak memory use is limited to a narrower window.

In the future I want to add a JSONGenerator::DumpEscaped() method that formats its output following the binary escape protocol. Every packet that sends JSON responses must escape it before sending, so there's no point in even generating the un-escaped version in the first place. It forces us to always have two copies of the string in heap for at least a little while.

[JDevlieghere]

If debugserver linked against libSupport we could have saved the additional copy altogether by using llvm::raw_string_ostream:

std::string str;
llvm::raw_string_ostream stream(str);
stream.str() // Flushes and returns a reference to the stack allocated str

Barring that, I wonder if if a little wrapper around std::ostringstream could improve readability and avoid bugs where someone forgets to call stream.str("").

class AggressiveStream {
public:
  AggressiveStream() : m_stream(std::make_unique<std::ostringstream>()) {}

  std::ostringstream &operator*() {
    assert(m_stream && "cannot use stream after having called str()");
    return *m_stream;
  }

  std::string str() {
    std::string s = m_stream->str();
    m_stream.reset();
    return std::move(s);
  }

private:
  std::unique_ptr<std::ostringstream> m_stream;
};

WDYT?

lldb/tools/debugserver/source/RNBRemote.cpp

588

clear doesn't do what you think it does, it modifies the state flag which isn't relevant here.

[jasonmolenda]

lldb/tools/debugserver/source/RNBRemote.cpp

588

Yeah I know, the common idiom for clearing an ostringstream is to .str("") an then .clear() to clear flag state, I was only clear'ing to be proper, even though we don't use the object past this point.

[jasonmolenda]

In D122848#3420581, @JDevlieghere wrote:

If debugserver linked against libSupport we could have saved the additional copy altogether by using llvm::raw_string_ostream:

std::string str;
llvm::raw_string_ostream stream(str);
stream.str() // Flushes and returns a reference to the stack allocated str

Barring that, I wonder if if a little wrapper around std::ostringstream could improve readability and avoid bugs where someone forgets to call stream.str("").

class AggressiveStream {
public:
  AggressiveStream() : m_stream(std::make_unique<std::ostringstream>()) {}

  std::ostringstream &operator*() {
    assert(m_stream && "cannot use stream after having called str()");
    return *m_stream;
  }

  std::string str() {
    std::string s = m_stream->str();
    m_stream.reset();
    return std::move(s);
  }

private:
  std::unique_ptr<std::ostringstream> m_stream;
};

WDYT?

That's a cool idea, and certainly less error prone than this by-hand method I did, but I think I'd rather fix JSONGenerator::Dump to return quoted strings so we don't need to immediately copy the entire string into another string for quoting. I don't feel strongly about it tho. I might be too optimistic that I'm going to fix Dump and it would be good to adopt a safer wrapper around ostringsream.

[jasonmolenda]

I was chatting with Jonas about this a bit. The part of this patch which fixes all of the SendPacket(outbuf.c_str()) calls, to avoid a temporary copy of the std::string, is clearly necessary and correct. The changes to clear the ostringstreams eagerly after we've escaped the data is noisy, and I'll remove those and look at updating JSONGenerator to print escaped string representations directly, to eliminate the unescaped copy & the escaped copy duplication. I'll update the patch in a bit.

[jasonmolenda]

Address Jonas' feedback by removing the parts of the patch that freed the ostringstream's after binary-escaped versions of them had been generated. Instead I'll add DumpEscaped methods to the JSONGenerator class.

lldb/tools/debugserver/source/RNBRemote.cpp

RNBRemote::SendAsyncJSONPacket(const JSONGenerator::Dictionary &dictionary) {
   std::ostringstream stream;
   // We're choosing something that is easy to spot if we somehow get one
   // of these coming out at the wrong time (i.e. when the remote side
   // is not waiting for a process control completion response).
   stream << "JSON-async:";
   dictionary.Dump(stream);
   const std::string payload = binary_encode_string(stream.str());
   return SendPacket(payload);
 }
 
 // Given a std::string packet contents to send, possibly encode/compress it.
 // If compression is enabled, the returned std::string will be in one of two
 // forms:
 //
 //    N<original packet contents uncompressed>
 //    C<size of original decompressed packet>:<packet compressed with the
 //    requested compression scheme>
if (strcmp(p, "vCont;c") == 0) {
 
     if (DNBDebugserverIsTranslated()) {
       DNBLogError("debugserver is x86_64 binary running in translation, attach "
                   "failed.");
       std::string return_message = "E96;";
       return_message +=
           cstring_to_asciihex_string("debugserver is x86_64 binary running in "
                                      "translation, attached failed.");
-      SendPacket(return_message.c_str());
+      SendPacket(return_message);
       return rnb_err;
     }
 
     if (strstr(p, "vAttachWait;") == p) {
       p += strlen("vAttachWait;");
       if (!GetProcessNameFrom_vAttach(p, attach_name)) {
         return HandlePacket_ILLFORMED(
             __FILE__, __LINE__, p, "non-hex char in arg on 'vAttachWait' pkt");
if (attach_pid != INVALID_NUB_PROCESS) {
       // string to lldb.
 
       if (pid_attaching_to != INVALID_NUB_PROCESS) {
         // The order of these checks is important.  
         if (process_does_not_exist (pid_attaching_to)) {
           DNBLogError("Tried to attach to pid that doesn't exist");
           std::string return_message = "E96;";
           return_message += cstring_to_asciihex_string("no such process.");
-          return SendPacket(return_message.c_str());
+          return SendPacket(return_message);
         }
         if (process_is_already_being_debugged (pid_attaching_to)) {
           DNBLogError("Tried to attach to process already being debugged");
           std::string return_message = "E96;";
           return_message += cstring_to_asciihex_string("tried to attach to "
                                            "process already being debugged");
-          return SendPacket(return_message.c_str());
+          return SendPacket(return_message);
         }
         uid_t my_uid, process_uid;
         if (attach_failed_due_to_uid_mismatch (pid_attaching_to, 
                                                my_uid, process_uid)) {
           std::string my_username = "uid " + std::to_string (my_uid);
           std::string process_username = "uid " + std::to_string (process_uid);
           struct passwd *pw = getpwuid (my_uid);
           if (pw && pw->pw_name) {
             my_username = pw->pw_name;
           }
           pw = getpwuid (process_uid);
           if (pw && pw->pw_name) {
             process_username = pw->pw_name;
           }
           DNBLogError("Tried to attach to process with uid mismatch");
           std::string return_message = "E96;";
           std::string msg = "tried to attach to process as user '" 
                             + my_username + "' and process is running "
                             "as user '" + process_username + "'";
           return_message += cstring_to_asciihex_string(msg.c_str());
-          return SendPacket(return_message.c_str());
+          return SendPacket(return_message);
         }
         if (!login_session_has_gui_access() && !developer_mode_enabled()) {
           DNBLogError("Developer mode is not enabled and this is a "
                       "non-interactive session");
           std::string return_message = "E96;";
           return_message += cstring_to_asciihex_string("developer mode is "
                                            "not enabled on this machine "
                                            "and this is a non-interactive "
                                            "debug session.");
-          return SendPacket(return_message.c_str());
+          return SendPacket(return_message);
         }
         if (!login_session_has_gui_access()) {
           DNBLogError("This is a non-interactive session");
           std::string return_message = "E96;";
           return_message += cstring_to_asciihex_string("this is a "
                                            "non-interactive debug session, "
                                            "cannot get permission to debug "
                                            "processes.");
-          return SendPacket(return_message.c_str());
+          return SendPacket(return_message);
         }
       }
 
       std::string error_explainer = "attach failed";
       if (err_str[0] != '\0') {
         // This is not a super helpful message for end users
         if (strcmp (err_str, "unable to start the exception thread") == 0) {
           snprintf (err_str, sizeof (err_str) - 1,
                     "Not allowed to attach to process.  Look in the console "
                     "messages (Console.app), near the debugserver entries, "
                     "when the attach failed.  The subsystem that denied "
                     "the attach permission will likely have logged an "
                     "informative message about why it was denied.");
           err_str[sizeof (err_str) - 1] = '\0';
         }
         error_explainer += " (";
         error_explainer += err_str;
         error_explainer += ")";
       }
       std::string default_return_msg = "E96;";
       default_return_msg += cstring_to_asciihex_string 
                               (error_explainer.c_str());
-      SendPacket (default_return_msg.c_str());
+      SendPacket (default_return_msg);
       DNBLogError("Attach failed: \"%s\".", err_str);
       return rnb_err;
     }
   }
 
   // All other failures come through here
   return HandlePacket_UNIMPLEMENTED(p);
 }
if (name == "scan_type") {
       if (iss >> std::hex >> int_value) {
         scan_type = (DNBProfileDataScanType)int_value;
       }
     }
   }
 
   std::string data = DNBProcessGetProfileData(pid, scan_type);
   if (!data.empty()) {
-    return SendPacket(data.c_str());
+    return SendPacket(data);
   } else {
     return SendPacket("OK");
   }
 }
 
 // QSetEnableAsyncProfiling;enable:[0|1]:interval_usec:XXXXXX;scan_type:0xYYYYYYY
 rnb_err_t RNBRemote::HandlePacket_SetEnableAsyncProfiling(const char *p) {
   nub_process_t pid = m_ctx.ProcessID();
rnb_err_t RNBRemote::HandlePacket_jThreadsInfo(const char *p) {
   if (m_ctx.HasValidProcessID()) {
     const bool threads_with_valid_stop_info_only = false;
     JSONGenerator::ObjectSP threads_info_sp =
         GetJSONThreadsInfo(threads_with_valid_stop_info_only);
 
     if (threads_info_sp) {
       std::ostringstream strm;
       threads_info_sp->Dump(strm);
+      threads_info_sp->Clear();
       std::string binary_packet = binary_encode_string(strm.str());
       if (!binary_packet.empty())
-        return SendPacket(binary_packet.c_str());
+        return SendPacket(binary_packet);
     }
   }
   return SendPacket("E85");
 }
 
 rnb_err_t RNBRemote::HandlePacket_jThreadExtendedInfo(const char *p) {
   nub_process_t pid;
   std::ostringstream json;
if (get_boolean_value_for_key_name_from_json("fetch_all_solibs", p,
         json_sp = DNBGetLoadedDynamicLibrariesInfos(pid, image_list_address,
                                                     image_count);
       }
     }
 
     if (json_sp.get()) {
       std::ostringstream json_str;
       json_sp->Dump(json_str);
+      json_sp->Clear();
       if (json_str.str().size() > 0) {
         std::string json_str_quoted = binary_encode_string(json_str.str());
-        return SendPacket(json_str_quoted.c_str());
+        return SendPacket(json_str_quoted);
       } else {
         SendPacket("E84");
       }
     }
   }
   return SendPacket("OK");
 }
 
rnb_err_t RNBRemote::HandlePacket_jGetSharedCacheInfo(const char *p) {
   const char get_shared_cache_info_str[] = {"jGetSharedCacheInfo:{"};
   if (strncmp(p, get_shared_cache_info_str,
               sizeof(get_shared_cache_info_str) - 1) == 0) {
     JSONGenerator::ObjectSP json_sp = DNBGetSharedCacheInfo(pid);
 
     if (json_sp.get()) {
       std::ostringstream json_str;
       json_sp->Dump(json_str);
+      json_sp->Clear();
       if (json_str.str().size() > 0) {
         std::string json_str_quoted = binary_encode_string(json_str.str());
-        return SendPacket(json_str_quoted.c_str());
+        return SendPacket(json_str_quoted);
       } else {
         SendPacket("E86");
       }
     }
   }
   return SendPacket("OK");
 }
 
rnb_err_t RNBRemote::HandlePacket_qSymbol(const char *command) {
   if (symbol_name.empty()) {
     // Done with symbol lookups
     return SendPacket("OK");
   } else {
     std::ostringstream reply;
     reply << "qSymbol:";
     for (size_t i = 0; i < symbol_name.size(); ++i)
       reply << RAWHEX8(symbol_name[i]);
-    return SendPacket(reply.str().c_str());
+    return SendPacket(reply.str());
   }
 }
 
 // Note that all numeric values returned by qProcessInfo are hex encoded,
 // including the pid and the cpu type.
 
 rnb_err_t RNBRemote::HandlePacket_qProcessInfo(const char *p) {
   nub_process_t pid;