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

105

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

29

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

31

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

33

Revert to maintain API

35–37

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

37

ditto, and revert for API

include/lldb/Target/Process.h

2084

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–25

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

Don't remove... API

39–44

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

source/API/SBMemoryRegionInfoList.cpp

81

revert

85–88

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

90

revert

110

remove const

115

revert

source/API/SBProcess.cpp

1367

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

1369

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

1371

If above changes are made this is not needed.

1399

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

1400

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

1401

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

1402–1403

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

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

31

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

31

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

31

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).

35–37

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

2084

Hah, I feel stupid now)

source/API/SBMemoryRegionInfo.cpp

23–25

This is instead of private constructor with a raw pointer

23–27

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

0

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

1

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

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

include/lldb/API/SBMemoryRegionInfoList.h

public:
   SBMemoryRegionInfoList();
 
   SBMemoryRegionInfoList(const lldb::SBMemoryRegionInfoList &rhs);
 
   const SBMemoryRegionInfoList &operator=(const SBMemoryRegionInfoList &rhs);
 
   ~SBMemoryRegionInfoList();
 
   uint32_t GetSize() const;
 
   bool GetMemoryRegionAtIndex(uint32_t idx, SBMemoryRegionInfo &region_info);
 
-  void Append(lldb::SBMemoryRegionInfo &region);
+  void Append(const lldb::SBMemoryRegionInfo &region);
 
-  void Append(lldb::SBMemoryRegionInfoList &region_list);
+  void Append(const lldb::SBMemoryRegionInfoList &region_list);
+
+  void Reserve(size_t capacity);
 
   void Clear();
 
 protected:
   const MemoryRegionInfoListImpl *operator->() const;
 
   const MemoryRegionInfoListImpl &operator*() const;
 
 private:
   std::unique_ptr<MemoryRegionInfoListImpl> m_opaque_ap;
 };
 
 } // namespace lldb
 
 #endif // LLDB_SBMemoryRegionInfoList_h_

include/lldb/Target/Process.h

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(std::vector<lldb_private::MemoryRegionInfo> &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.
 //
 //===----------------------------------------------------------------------===//
 

 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/SBMemoryRegionInfo.cpp

 #include "lldb/Utility/StreamString.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 SBMemoryRegionInfo::SBMemoryRegionInfo()
     : m_opaque_ap(new MemoryRegionInfo()) {}
 
-SBMemoryRegionInfo::SBMemoryRegionInfo(const MemoryRegionInfo *lldb_object_ptr)
-    : m_opaque_ap(new MemoryRegionInfo()) {
-  if (lldb_object_ptr)
-    ref() = *lldb_object_ptr;
-}
+SBMemoryRegionInfo::SBMemoryRegionInfo(MemoryRegionInfo lldb_object)
+    : m_opaque_ap(new MemoryRegionInfo(std::move(lldb_object))) {}
 
 SBMemoryRegionInfo::SBMemoryRegionInfo(const SBMemoryRegionInfo &rhs)
     : m_opaque_ap(new MemoryRegionInfo()) {
   ref() = rhs.ref();
 }
 
 const SBMemoryRegionInfo &SBMemoryRegionInfo::
 operator=(const SBMemoryRegionInfo &rhs) {
   if (this != &rhs) {
     ref() = rhs.ref();
   }
   return *this;
 }
 
 SBMemoryRegionInfo::~SBMemoryRegionInfo() {}
 
 void SBMemoryRegionInfo::Clear() { m_opaque_ap->Clear(); }
 
 bool SBMemoryRegionInfo::operator==(const SBMemoryRegionInfo &rhs) const {
   return ref() == rhs.ref();
 }
 
 bool SBMemoryRegionInfo::operator!=(const SBMemoryRegionInfo &rhs) const {
   return ref() != rhs.ref();
 }
 
 MemoryRegionInfo &SBMemoryRegionInfo::ref() { return *m_opaque_ap; }
 

source/API/SBMemoryRegionInfoList.cpp

public:
 
   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 Reserve(size_t capacity) { return m_regions.reserve(capacity); }
 
   void Append(const lldb::SBMemoryRegionInfo &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,
+  bool GetMemoryRegionInfoAtIndex(size_t index,
                                   SBMemoryRegionInfo &region_info) {
     if (index >= GetSize())
       return false;
     region_info = m_regions[index];
     return true;
   }
 
 private:

 const SBMemoryRegionInfoList &SBMemoryRegionInfoList::
 operator=(const SBMemoryRegionInfoList &rhs) {
   if (this != &rhs) {
     *m_opaque_ap = *rhs.m_opaque_ap;
   }
   return *this;
 }
 
 uint32_t SBMemoryRegionInfoList::GetSize() const {
   return m_opaque_ap->GetSize();
 }
 
+void SBMemoryRegionInfoList::Reserve(size_t capacity) {
+  m_opaque_ap->Reserve(capacity);
+}
+
 bool SBMemoryRegionInfoList::GetMemoryRegionAtIndex(
     uint32_t idx, SBMemoryRegionInfo &region_info) {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
 
   bool result = m_opaque_ap->GetMemoryRegionInfoAtIndex(idx, region_info);
 
   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) {
+void SBMemoryRegionInfoList::Append(const SBMemoryRegionInfo &sb_region) {
   m_opaque_ap->Append(sb_region);
 }
 
-void SBMemoryRegionInfoList::Append(SBMemoryRegionInfoList &sb_region_list) {
+void
+SBMemoryRegionInfoList::Append(const 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

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());
+
+      MemoryRegionInfo region_info;
       sb_error.ref() =
-          process_sp->GetMemoryRegionInfo(load_addr, *region_info_sp);
+          process_sp->GetMemoryRegionInfo(load_addr, region_info);
       if (sb_error.Success()) {
-        sb_region_info.ref() = *region_info_sp;
+        sb_region_info.ref() = std::move(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");
   }
   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)
+    return sb_region_list;
+
+
+    std::vector<MemoryRegionInfo> region_list;
+    if (process_sp->GetMemoryRegions(region_list).Success()) {
+      sb_region_list.Reserve(sb_region_list.GetSize() + region_list.size());
+      for (auto& region : region_list)
+        sb_region_list.Append(SBMemoryRegionInfo(std::move(region)));
   }
+
   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(std::vector<lldb_private::MemoryRegionInfo> &region_list) {
 
   Status error;
 
   lldb::addr_t range_end = 0;
 
   region_list.clear();
   do {
-    lldb::MemoryRegionInfoSP region_info(new lldb_private::MemoryRegionInfo());
+    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);
+      region_list.push_back(std::move(region_info));
     }
   } while (range_end != LLDB_INVALID_ADDRESS);
 
   return error;
 }
 
 Status
 Process::ConfigureStructuredData(const ConstString &type_name,