Speadup memory regions handling and cleanup related code

[tatyana-krasnukha]

Summary

• replace unnecessary shared pointers with unique pointers
• reserve space before filling a vector with 'push_back' in a loop to avoid muptiple allocations and memory fragmentation (except Process::GetMemoryRegions, it is not possible to know the size in advance here)
• override GetMemoryRegions for the ProcessMinidump:

Process::GetMemoryRegions fills the list of regions by calling GetMemoryRegionInfo until it returns an error. But ProcessMinidump implementation GetMemoryRegionInfo parses whole list every time and searches requested range there. Now GetMemoryRegions does this work just once.

• use move semantic where it is possible (and desirable)
• fix size_t -> uint32_t truncation
• add missing constness

I had to change API slightly, but believe these changes doesn't break existing code that uses this API.
Profiling a sipmle program I got nice results - execution of GetMemoryRegions function took just 0.01% of total time against 0.17% for unchanged version.

[labath]

Woohoo for speedups.

However, I noticed that you are modifying the signatures of SB functions, which I don't believe will be acceptable for lldb.

include/lldb/API/SBMemoryRegionInfo.h

1

Generally, I think it's better to use values here instead of rvalue references, as they make clearer interfaces (a function which receives the a unique_ptr by value has to consume it whether it likes to or not, but in case of rvalue references, the object will survive the function call unless the function explicitly does something to it).

I don't believe this should matter for performance, as that's just copying the pointer value around.

source/Plugins/Process/minidump/MinidumpParser.cpp

404 ↗

(On Diff #177326)

Namespaces are not indented in llvm style.

408 ↗

(On Diff #177326)

llvm prefers static functions instead of ones in anonymous namespaces

[clayborg]

I am going to stop making comments as I have been working on a similar patch that does more than this one. I will post it today and we can decide which approach we want to use.

include/lldb/API/SBMemoryRegionInfoList.h

6–1

Revert. You can add functions, but never change them. Our API is stable.

6–1

This you can add, but std::vector implementations are quite efficient and I doubt this is really needed?

6–1

Revert to maintain API

6–1

"const" means nothing to our objects as they have shared pointers or unique pointers which never change.

8

ditto, and revert for API

include/lldb/Target/Process.h

0–1

This was a a vector of shared pointers, seemingly for no good reason prior to this change, I would change this to be a vector of MemoryRegionInfo instances?

source/API/SBMemoryRegionInfo.cpp

23–24 ↗

(On Diff #177326)

Fine to add to API, but really no one outside LLDB will be able to do anything with this function (nor the one on the left) since MemoryRegionInfo is opaque as far as the public clients are concerned. It is there for internal LLDB convenience. So do we really need this? We can add a method that copies from an instance if needed. MemoryRegionInfo structs are very simple to copy. No need for heroics here.

23–27 ↗

(On Diff #177326)

Don't remove... API

39–44 ↗

(On Diff #177326)

Do this by instance instead of a move assignment operator with a unique ptr?

source/API/SBMemoryRegionInfoList.cpp

16–1

revert

16–1

revert

16–1

remove const

17

revert

17–20

Who needs this externally? I would rather not add this to the public API?

source/API/SBProcess.cpp

-5–-4

This doesn't need to be done if the above code is changed to just pass the vector of regions from "sb_region_list"

-6

This doesn't need to be done if the above code is changed

-7

Change Process:: GetMemoryRegions() to take a "std::vector<MemoryRegionInfo>&region_list"?

-8

Just use the std::vector<MemoryRegionInfo> from MemoryRegionInfoListImpl inside the "sb_region_list" already?

-14

Remove. "sb_region_info" already has an instance we can use.

-16–1

Change Process::GetMemoryRegionInfo() to use an "MemoryRegionInfo &region" instead of shared pointer or unique pointer?

-16–1

If above changes are made this is not needed.

source/Plugins/Process/minidump/MinidumpParser.h

91 ↗

(On Diff #177326)

switch to all internal GetMemoryRegions calls to just use "std::vector<lldb::MemoryRegionInfo>"?

[zturner]

include/lldb/API/SBMemoryRegionInfoList.h

6–1

std::vector is as efficient as it can be without additional hints from the user, but reserve is a very useful optimization in general.

[clayborg]

See my patch: https://reviews.llvm.org/D55522

Let me know what you think

[clayborg]

include/lldb/API/SBMemoryRegionInfoList.h

6–1

Agreed but this doesn't need to be in the public API as there is no real need.

[tatyana-krasnukha]

Thanks to all for comments!

As I wrote in inline comments, 'non-const' -> 'const' changes don't seem to break anything, I'd like to leave it here. Correct me, if I'm wrong.

The second question is whether it is allowed to change private member functions of SBXxx classes?

include/lldb/API/SBMemoryRegionInfoList.h

6–1

I'm not aware of the usual count of memory regions in dump files, but when I tested on a file with 140 regions the difference was significant
(if take into account all loops that do push_back without previous reserve).

6–1

Non-const argument version prevents passing r-value in this function. Another reason is readability.
Is this change braking API too?

include/lldb/Target/Process.h

0–1

Hah, I feel stupid now)

source/API/SBMemoryRegionInfo.cpp

23–24 ↗

(On Diff #177326)

This is instead of private constructor with a raw pointer

23–27 ↗

(On Diff #177326)

Private member functions cannot be changed too?

[tatyana-krasnukha]

Replace vector of unique pointers with vector of values, revert some API changes, remove MinidumpParser changes.
I will put overridden GetMemoryRegions in a separate patch.

[tatyana-krasnukha]

Addressed more comments

[tatyana-krasnukha]

Minor change: replace using with typedef since old swig versions don't understand c++11's aliases.

[clayborg]

One quick change required to avoid STL in public headers.

include/lldb/lldb-forward.h

16

Remove due to this header being part of the public API.

135

After thinking about this a bit, we shouldn't put any STL stuff in headers that are part of the public API as we never want to expose any STL through our API. Can probably move this into MemoryRegionInfo.h, or just use "std::vector<lldb_private::MemoryRegionInfo>" everywhere as long as it is in lldb_private or the global namespace (we only export "lldb::*").

[tatyana-krasnukha]

Rid off of including std::vector at the cost of adding a wrapper class that can be forward declared.
Without this, it is not possible to do https://reviews.llvm.org/D55472#inline-491159

include/lldb/API/SBMemoryRegionInfo.h

 //===-- SBMemoryRegionInfo.h ------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 

 private:
   friend class SBProcess;
   friend class SBMemoryRegionInfoList;
 
   lldb_private::MemoryRegionInfo &ref();
 
   const lldb_private::MemoryRegionInfo &ref() const;
 
+  // Unused.
   SBMemoryRegionInfo(const lldb_private::MemoryRegionInfo *lldb_object_ptr);
 
   lldb::MemoryRegionInfoUP m_opaque_ap;
 };
 
 } // namespace lldb
 
 #endif // LLDB_SBMemoryRegionInfo_h_

include/lldb/API/SBMemoryRegionInfoList.h

 //===-- SBMemoryRegionInfoList.h --------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SBMemoryRegionInfoList_h_
 #define LLDB_SBMemoryRegionInfoList_h_
 
 #include "lldb/API/SBDefines.h"
 
 class MemoryRegionInfoListImpl;
 
public:
   void Clear();
 
 protected:
   const MemoryRegionInfoListImpl *operator->() const;
 
   const MemoryRegionInfoListImpl &operator*() const;
 
 private:
+  friend class SBProcess;
+
+  lldb_private::MemoryRegionInfos &ref();
+
+  const lldb_private::MemoryRegionInfos &ref() const;
+
   std::unique_ptr<MemoryRegionInfoListImpl> m_opaque_ap;
 };
 
 } // namespace lldb
 
 #endif // LLDB_SBMemoryRegionInfoList_h_

include/lldb/Target/MemoryRegionInfo.h

 inline bool operator<(const MemoryRegionInfo &lhs, lldb::addr_t rhs) {
   return lhs.GetRange().GetRangeBase() < rhs;
 }
 
 inline bool operator<(lldb::addr_t lhs, const MemoryRegionInfo &rhs) {
   return lhs < rhs.GetRange().GetRangeBase();
 }
 
+// Forward-declarable wrapper.
+class MemoryRegionInfos : public std::vector<lldb_private::MemoryRegionInfo> {
+public:
+  using std::vector<lldb_private::MemoryRegionInfo>::vector;
+};
+
 }
 
 namespace llvm {
 template <>
 struct format_provider<lldb_private::MemoryRegionInfo::OptionalBool> {
   static void format(const lldb_private::MemoryRegionInfo::OptionalBool &B,
                      raw_ostream &OS, StringRef Options) {
     switch(B) {

include/lldb/Target/Process.h

 //===-- Process.h -----------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
public:
   /// @param[out] region_list
   ///     A vector to contain MemoryRegionInfo objects for all mapped
   ///     ranges.
   ///
   /// @return
   ///     An error value.
   //------------------------------------------------------------------
   virtual Status
-  GetMemoryRegions(std::vector<lldb::MemoryRegionInfoSP> &region_list);
+  GetMemoryRegions(lldb_private::MemoryRegionInfos &region_list);
 
   virtual Status GetWatchpointSupportInfo(uint32_t &num) {
     Status error;
     num = 0;
     error.SetErrorString("Process::GetWatchpointSupportInfo() not supported");
     return error;
   }
 

include/lldb/lldb-forward.h

 //===-- lldb-forward.h ------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_lldb_forward_h_
 #define LLDB_lldb_forward_h_
 
 #if defined(__cplusplus)
 
 #include "lldb/Utility/SharingPtr.h"
 
 //----------------------------------------------------------------------
 // lldb forward declarations
 //----------------------------------------------------------------------
 namespace lldb_private {
 
 class ABI;
 class Address;
 class AddressImpl;

 class IOHandler;
 class IOObject;
 class IRExecutionUnit;
 class JITLoader;
 class JITLoaderList;
 class Language;
 class LanguageCategory;
 class LanguageRuntime;
-class MemoryRegionInfo;
 class LineTable;
 class Listener;
 class Log;
 class Mangled;
 class Materializer;
 class MemoryHistory;
+class MemoryRegionInfo;
+class MemoryRegionInfos;
 class Module;
 class ModuleList;
 class ModuleSpec;
 class ModuleSpecList;
 class Mutex;
 struct NameSearchContext;
 class ObjCLanguageRuntime;
 class ObjCRuntimeSyntheticProvider;

 typedef std::unique_ptr<lldb_private::JITLoaderList> JITLoaderListUP;
 typedef std::shared_ptr<lldb_private::LanguageRuntime> LanguageRuntimeSP;
 typedef std::shared_ptr<lldb_private::SystemRuntime> SystemRuntimeSP;
 typedef std::unique_ptr<lldb_private::SystemRuntime> SystemRuntimeUP;
 typedef std::shared_ptr<lldb_private::LineTable> LineTableSP;
 typedef std::shared_ptr<lldb_private::Listener> ListenerSP;
 typedef std::weak_ptr<lldb_private::Listener> ListenerWP;
 typedef std::shared_ptr<lldb_private::MemoryHistory> MemoryHistorySP;
-typedef std::shared_ptr<lldb_private::MemoryRegionInfo> MemoryRegionInfoSP;
 typedef std::unique_ptr<lldb_private::MemoryRegionInfo> MemoryRegionInfoUP;
 typedef std::shared_ptr<lldb_private::Module> ModuleSP;
 typedef std::weak_ptr<lldb_private::Module> ModuleWP;
 typedef std::shared_ptr<lldb_private::ObjectFile> ObjectFileSP;
 typedef std::weak_ptr<lldb_private::ObjectFile> ObjectFileWP;
 typedef std::shared_ptr<lldb_private::ObjectFileJITDelegate>
     ObjectFileJITDelegateSP;
 typedef std::weak_ptr<lldb_private::ObjectFileJITDelegate>

source/API/SBMemoryRegionInfoList.cpp

 //===-- SBMemoryRegionInfoList.cpp ------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/API/SBMemoryRegionInfoList.h"
 #include "lldb/API/SBMemoryRegionInfo.h"
 #include "lldb/API/SBStream.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Utility/Log.h"
 
 #include <vector>
 
 using namespace lldb;
 using namespace lldb_private;
 
 class MemoryRegionInfoListImpl {
 public:
   MemoryRegionInfoListImpl() : m_regions() {}
 
   MemoryRegionInfoListImpl(const MemoryRegionInfoListImpl &rhs)
       : m_regions(rhs.m_regions) {}
 
   MemoryRegionInfoListImpl &operator=(const MemoryRegionInfoListImpl &rhs) {
     if (this == &rhs)
       return *this;
     m_regions = rhs.m_regions;
     return *this;
   }
 
-  uint32_t GetSize() { return m_regions.size(); }
+  size_t GetSize() const { return m_regions.size(); }
 
-  void Append(const lldb::SBMemoryRegionInfo &sb_region) {
+  void Reserve(size_t capacity) { return m_regions.reserve(capacity); }
+
+  void Append(const MemoryRegionInfo &sb_region) {
     m_regions.push_back(sb_region);
   }
 
   void Append(const MemoryRegionInfoListImpl &list) {
-    for (auto val : list.m_regions)
+    Reserve(GetSize() + list.GetSize());
+
+    for (const auto &val : list.m_regions)
       Append(val);
   }
 
   void Clear() { m_regions.clear(); }
 
-  bool GetMemoryRegionInfoAtIndex(uint32_t index,
-                                  SBMemoryRegionInfo &region_info) {
+  bool GetMemoryRegionInfoAtIndex(size_t index,
+                                  MemoryRegionInfo &region_info) {
     if (index >= GetSize())
       return false;
     region_info = m_regions[index];
     return true;
   }
 
+  MemoryRegionInfos &Ref() { return m_regions; }
+
+  const MemoryRegionInfos &Ref() const { return m_regions; }
+
 private:
-  std::vector<lldb::SBMemoryRegionInfo> m_regions;
+  MemoryRegionInfos m_regions;
 };
 
+MemoryRegionInfos &SBMemoryRegionInfoList::ref() {
+  return m_opaque_ap->Ref();
+}
+
+const MemoryRegionInfos &SBMemoryRegionInfoList::ref() const {
+  return m_opaque_ap->Ref();
+}
+
 SBMemoryRegionInfoList::SBMemoryRegionInfoList()
     : m_opaque_ap(new MemoryRegionInfoListImpl()) {}
 
 SBMemoryRegionInfoList::SBMemoryRegionInfoList(
     const SBMemoryRegionInfoList &rhs)
     : m_opaque_ap(new MemoryRegionInfoListImpl(*rhs.m_opaque_ap)) {}
 
 SBMemoryRegionInfoList::~SBMemoryRegionInfoList() {}

 uint32_t SBMemoryRegionInfoList::GetSize() const {
   return m_opaque_ap->GetSize();
 }
 
 bool SBMemoryRegionInfoList::GetMemoryRegionAtIndex(
     uint32_t idx, SBMemoryRegionInfo &region_info) {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
 
-  bool result = m_opaque_ap->GetMemoryRegionInfoAtIndex(idx, region_info);
+  bool result = m_opaque_ap->GetMemoryRegionInfoAtIndex(idx, region_info.ref());
 
   if (log) {
     SBStream sstr;
     region_info.GetDescription(sstr);
     log->Printf("SBMemoryRegionInfoList::GetMemoryRegionAtIndex (this.ap=%p, "
                 "idx=%d) => SBMemoryRegionInfo (this.ap=%p, '%s')",
                 static_cast<void *>(m_opaque_ap.get()), idx,
                 static_cast<void *>(region_info.m_opaque_ap.get()),
                 sstr.GetData());
   }
 
   return result;
 }
 
 void SBMemoryRegionInfoList::Clear() { m_opaque_ap->Clear(); }
 
 void SBMemoryRegionInfoList::Append(SBMemoryRegionInfo &sb_region) {
-  m_opaque_ap->Append(sb_region);
+  m_opaque_ap->Append(sb_region.ref());
 }
 
 void SBMemoryRegionInfoList::Append(SBMemoryRegionInfoList &sb_region_list) {
   m_opaque_ap->Append(*sb_region_list);
 }
 
 const MemoryRegionInfoListImpl *SBMemoryRegionInfoList::operator->() const {
   return m_opaque_ap.get();
 }
 
 const MemoryRegionInfoListImpl &SBMemoryRegionInfoList::operator*() const {
   assert(m_opaque_ap.get());
   return *m_opaque_ap.get();
 }

source/API/SBProcess.cpp

 //===-- SBProcess.cpp -------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
lldb::SBError SBProcess::SaveCore(const char *file_name) {
   return error;
 }
 
 lldb::SBError
 SBProcess::GetMemoryRegionInfo(lldb::addr_t load_addr,
                                SBMemoryRegionInfo &sb_region_info) {
   lldb::SBError sb_error;
   ProcessSP process_sp(GetSP());
-  MemoryRegionInfoSP region_info_sp =
-      std::make_shared<lldb_private::MemoryRegionInfo>();
   if (process_sp) {
     Process::StopLocker stop_locker;
     if (stop_locker.TryLock(&process_sp->GetRunLock())) {
       std::lock_guard<std::recursive_mutex> guard(
           process_sp->GetTarget().GetAPIMutex());
+
       sb_error.ref() =
-          process_sp->GetMemoryRegionInfo(load_addr, *region_info_sp);
-      if (sb_error.Success()) {
-        sb_region_info.ref() = *region_info_sp;
-      }
+          process_sp->GetMemoryRegionInfo(load_addr, sb_region_info.ref());
     } else {
       Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
       if (log)
         log->Printf(
             "SBProcess(%p)::GetMemoryRegionInfo() => error: process is running",
             static_cast<void *>(process_sp.get()));
       sb_error.SetErrorString("process is running");
     }
   } else {
     sb_error.SetErrorString("SBProcess is invalid");
   }
   return sb_error;
 }
 
 lldb::SBMemoryRegionInfoList SBProcess::GetMemoryRegions() {
-  lldb::SBError sb_error;
   lldb::SBMemoryRegionInfoList sb_region_list;
+
   ProcessSP process_sp(GetSP());
-  if (process_sp) {
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      std::lock_guard<std::recursive_mutex> guard(
-          process_sp->GetTarget().GetAPIMutex());
-      std::vector<MemoryRegionInfoSP> region_list;
-      sb_error.ref() = process_sp->GetMemoryRegions(region_list);
-      if (sb_error.Success()) {
-        std::vector<MemoryRegionInfoSP>::iterator end = region_list.end();
-        for (std::vector<MemoryRegionInfoSP>::iterator it = region_list.begin();
-             it != end; it++) {
-          SBMemoryRegionInfo sb_region_info(it->get());
-          sb_region_list.Append(sb_region_info);
-        }
-      }
-    } else {
-      Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
-      if (log)
-        log->Printf(
-            "SBProcess(%p)::GetMemoryRegionInfo() => error: process is running",
-            static_cast<void *>(process_sp.get()));
-      sb_error.SetErrorString("process is running");
-    }
-  } else {
-    sb_error.SetErrorString("SBProcess is invalid");
+  if (process_sp && stop_locker.TryLock(&process_sp->GetRunLock())) {
+
+    process_sp->GetMemoryRegions(sb_region_list.ref());
   }
+
   return sb_region_list;
 }
 
 lldb::SBProcessInfo SBProcess::GetProcessInfo() {
   lldb::SBProcessInfo sb_proc_info;
   ProcessSP process_sp(GetSP());
   ProcessInstanceInfo proc_info;
   if (process_sp && process_sp->GetProcessInfo(proc_info)) {
     sb_proc_info.SetProcessInfo(proc_info);
   }
   return sb_proc_info;
 }

source/Target/Process.cpp

if (next_branch_insn_address.IsValid() &&
       retval = next_branch_insn_address;
     }
   }
 
   return retval;
 }
 
 Status
-Process::GetMemoryRegions(std::vector<lldb::MemoryRegionInfoSP> &region_list) {
+Process::GetMemoryRegions(lldb_private::MemoryRegionInfos &region_list) {
 
   Status error;
 
   lldb::addr_t range_end = 0;
 
   region_list.clear();
   do {
-    lldb::MemoryRegionInfoSP region_info(new lldb_private::MemoryRegionInfo());
-    error = GetMemoryRegionInfo(range_end, *region_info);
+    lldb_private::MemoryRegionInfo region_info;
+    error = GetMemoryRegionInfo(range_end, region_info);
     // GetMemoryRegionInfo should only return an error if it is unimplemented.
     if (error.Fail()) {
       region_list.clear();
       break;
     }
 
-    range_end = region_info->GetRange().GetRangeEnd();
-    if (region_info->GetMapped() == MemoryRegionInfo::eYes) {
-      region_list.push_back(region_info);
+    range_end = region_info.GetRange().GetRangeEnd();
+    if (region_info.GetMapped() == MemoryRegionInfo::eYes) {
+      region_list.push_back(std::move(region_info));
     }
   } while (range_end != LLDB_INVALID_ADDRESS);
 
   return error;
 }
 
 Status
 Process::ConfigureStructuredData(const ConstString &type_name,