tatyana-krasnukha created this revision.
tatyana-krasnukha added a reviewer: clayborg.
tatyana-krasnukha added a project: LLDB.
Herald added subscribers: lldb-commits, abidh.
- 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.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D55472
Files:
include/lldb/API/SBMemoryRegionInfo.h
include/lldb/API/SBMemoryRegionInfoList.h
include/lldb/Target/Process.h
include/lldb/lldb-forward.h
source/API/SBMemoryRegionInfo.cpp
source/API/SBMemoryRegionInfoList.cpp
source/API/SBProcess.cpp
source/Plugins/Process/minidump/MinidumpParser.cpp
source/Plugins/Process/minidump/MinidumpParser.h
source/Plugins/Process/minidump/MinidumpTypes.cpp
source/Plugins/Process/minidump/ProcessMinidump.cpp
source/Plugins/Process/minidump/ProcessMinidump.h
source/Target/Process.cpp
Index: source/Target/Process.cpp
===================================================================
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -6030,7 +6030,7 @@
}
Status
-Process::GetMemoryRegions(std::vector<lldb::MemoryRegionInfoSP> ®ion_list) {
+Process::GetMemoryRegions(std::vector<lldb::MemoryRegionInfoUP> ®ion_list) {
Status error;
@@ -6038,7 +6038,7 @@
region_list.clear();
do {
- lldb::MemoryRegionInfoSP region_info(new lldb_private::MemoryRegionInfo());
+ lldb::MemoryRegionInfoUP region_info(new lldb_private::MemoryRegionInfo());
error = GetMemoryRegionInfo(range_end, *region_info);
// GetMemoryRegionInfo should only return an error if it is unimplemented.
if (error.Fail()) {
@@ -6048,7 +6048,7 @@
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);
Index: source/Plugins/Process/minidump/ProcessMinidump.h
===================================================================
--- source/Plugins/Process/minidump/ProcessMinidump.h
+++ source/Plugins/Process/minidump/ProcessMinidump.h
@@ -78,6 +78,9 @@
Status GetMemoryRegionInfo(lldb::addr_t load_addr,
MemoryRegionInfo &range_info) override;
+ Status
+ GetMemoryRegions(std::vector<lldb::MemoryRegionInfoUP> ®ion_list) override;
+
bool GetProcessInfo(ProcessInstanceInfo &info) override;
Status WillResume() override {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===================================================================
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -294,6 +294,12 @@
return error;
}
+Status ProcessMinidump::GetMemoryRegions(
+ std::vector<lldb::MemoryRegionInfoUP> ®ion_list) {
+ region_list.clear();
+ return m_minidump_parser.GetMemoryRegions(region_list);
+}
+
void ProcessMinidump::Clear() { Process::m_thread_list.Clear(); }
bool ProcessMinidump::UpdateThreadList(ThreadList &old_thread_list,
Index: source/Plugins/Process/minidump/MinidumpTypes.cpp
===================================================================
--- source/Plugins/Process/minidump/MinidumpTypes.cpp
+++ source/Plugins/Process/minidump/MinidumpTypes.cpp
@@ -242,6 +242,8 @@
return {};
std::vector<const MinidumpMemoryInfo *> result;
+ result.reserve(header->num_of_entries);
+
for (uint64_t i = 0; i < header->num_of_entries; ++i) {
result.push_back(reinterpret_cast<const MinidumpMemoryInfo *>(
data.data() + i * header->size_of_entry));
Index: source/Plugins/Process/minidump/MinidumpParser.h
===================================================================
--- source/Plugins/Process/minidump/MinidumpParser.h
+++ source/Plugins/Process/minidump/MinidumpParser.h
@@ -88,6 +88,8 @@
llvm::Optional<MemoryRegionInfo> GetMemoryRegionInfo(lldb::addr_t);
+ Status GetMemoryRegions(std::vector<lldb::MemoryRegionInfoUP> ®ion_list);
+
// Perform consistency checks and initialize internal data structures
Status Initialize();
Index: source/Plugins/Process/minidump/MinidumpParser.cpp
===================================================================
--- source/Plugins/Process/minidump/MinidumpParser.cpp
+++ source/Plugins/Process/minidump/MinidumpParser.cpp
@@ -401,9 +401,40 @@
return range->range_ref.slice(offset, overlap);
}
+namespace {
+ constexpr uint32_t StateMemFree =
+ static_cast<uint32_t>(MinidumpMemoryInfoState::MemFree);
+
+ MemoryRegionInfo MemoryInfoToRegionInfo(const MinidumpMemoryInfo &entry) {
+ const auto yes = MemoryRegionInfo::eYes;
+ const auto no = MemoryRegionInfo::eNo;
+
+ const auto head = entry.base_address;
+ const auto tail = head + entry.region_size;
+ MemoryRegionInfo info;
+
+ info.GetRange().SetRangeBase(head);
+ info.GetRange().SetRangeEnd(tail);
+
+ const uint32_t PageNoAccess =
+ static_cast<uint32_t>(MinidumpMemoryProtectionContants::PageNoAccess);
+ info.SetReadable((entry.protect & PageNoAccess) == 0 ? yes : no);
+
+ const uint32_t PageWritable =
+ static_cast<uint32_t>(MinidumpMemoryProtectionContants::PageWritable);
+ info.SetWritable((entry.protect & PageWritable) != 0 ? yes : no);
+
+ const uint32_t PageExecutable = static_cast<uint32_t>(
+ MinidumpMemoryProtectionContants::PageExecutable);
+ info.SetExecutable((entry.protect & PageExecutable) != 0 ? yes : no);
+ info.SetMapped((entry.state != StateMemFree) ? yes : no);
+
+ return info;
+}
+} // namespace
+
llvm::Optional<MemoryRegionInfo>
MinidumpParser::GetMemoryRegionInfo(lldb::addr_t load_addr) {
- MemoryRegionInfo info;
llvm::ArrayRef<uint8_t> data = GetStream(MinidumpStreamType::MemoryInfoList);
if (data.empty())
return llvm::None;
@@ -417,33 +448,14 @@
const auto no = MemoryRegionInfo::eNo;
const MinidumpMemoryInfo *next_entry = nullptr;
- for (const auto &entry : mem_info_list) {
+ for (const auto entry : mem_info_list) {
const auto head = entry->base_address;
const auto tail = head + entry->region_size;
if (head <= load_addr && load_addr < tail) {
- info.GetRange().SetRangeBase(
- (entry->state != uint32_t(MinidumpMemoryInfoState::MemFree))
- ? head
- : load_addr);
- info.GetRange().SetRangeEnd(tail);
-
- const uint32_t PageNoAccess =
- static_cast<uint32_t>(MinidumpMemoryProtectionContants::PageNoAccess);
- info.SetReadable((entry->protect & PageNoAccess) == 0 ? yes : no);
-
- const uint32_t PageWritable =
- static_cast<uint32_t>(MinidumpMemoryProtectionContants::PageWritable);
- info.SetWritable((entry->protect & PageWritable) != 0 ? yes : no);
-
- const uint32_t PageExecutable = static_cast<uint32_t>(
- MinidumpMemoryProtectionContants::PageExecutable);
- info.SetExecutable((entry->protect & PageExecutable) != 0 ? yes : no);
-
- const uint32_t MemFree =
- static_cast<uint32_t>(MinidumpMemoryInfoState::MemFree);
- info.SetMapped((entry->state != MemFree) ? yes : no);
-
+ MemoryRegionInfo info = MemoryInfoToRegionInfo(*entry);
+ if (entry->state == StateMemFree)
+ info.GetRange().SetRangeBase(load_addr);
return info;
} else if (head > load_addr &&
(next_entry == nullptr || head < next_entry->base_address)) {
@@ -455,6 +467,7 @@
// No containing region found. Create an unmapped region that extends to the
// next region or LLDB_INVALID_ADDRESS
+ MemoryRegionInfo info;
info.GetRange().SetRangeBase(load_addr);
info.GetRange().SetRangeEnd((next_entry != nullptr) ? next_entry->base_address
: LLDB_INVALID_ADDRESS);
@@ -469,6 +482,27 @@
return info;
}
+Status MinidumpParser::GetMemoryRegions(
+ std::vector<lldb::MemoryRegionInfoUP> ®ion_list) {
+ llvm::ArrayRef<uint8_t> data = GetStream(MinidumpStreamType::MemoryInfoList);
+ if (data.empty())
+ return Status("No memory info data");
+
+ std::vector<const MinidumpMemoryInfo *> mem_info_list =
+ MinidumpMemoryInfo::ParseMemoryInfoList(data);
+ if (mem_info_list.empty())
+ return Status("Failed to parse memory info data");
+
+ region_list.reserve(mem_info_list.size());
+ for (const auto entry : mem_info_list) {
+ if (entry->state != StateMemFree)
+ region_list.emplace_back(
+ new MemoryRegionInfo(MemoryInfoToRegionInfo(*entry)));
+ }
+
+ return Status();
+}
+
Status MinidumpParser::Initialize() {
Status error;
Index: source/API/SBProcess.cpp
===================================================================
--- source/API/SBProcess.cpp
+++ source/API/SBProcess.cpp
@@ -1358,17 +1358,17 @@
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());
+
+ MemoryRegionInfoUP region_info_up(new lldb_private::MemoryRegionInfo());
sb_error.ref() =
- process_sp->GetMemoryRegionInfo(load_addr, *region_info_sp);
+ process_sp->GetMemoryRegionInfo(load_addr, *region_info_up);
if (sb_error.Success()) {
- sb_region_info.ref() = *region_info_sp;
+ sb_region_info.m_opaque_ap = std::move(region_info_up);
}
} else {
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
@@ -1385,35 +1385,31 @@
}
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");
+ if (!process_sp)
+ return sb_region_list;
+
+ Process::StopLocker stop_locker;
+ if (stop_locker.TryLock(&process_sp->GetRunLock())) {
+ std::lock_guard<std::recursive_mutex> guard(
+ process_sp->GetTarget().GetAPIMutex());
+
+ std::vector<MemoryRegionInfoUP> 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)));
}
} else {
- sb_error.SetErrorString("SBProcess is invalid");
+ 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()));
}
+
return sb_region_list;
}
Index: source/API/SBMemoryRegionInfoList.cpp
===================================================================
--- source/API/SBMemoryRegionInfoList.cpp
+++ source/API/SBMemoryRegionInfoList.cpp
@@ -32,20 +32,24 @@
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 ®ion_info) {
if (index >= GetSize())
return false;
@@ -74,12 +78,16 @@
return *this;
}
-uint32_t SBMemoryRegionInfoList::GetSize() const {
+size_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 ®ion_info) {
+ size_t idx, SBMemoryRegionInfo ®ion_info) {
Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
bool result = m_opaque_ap->GetMemoryRegionInfoAtIndex(idx, region_info);
@@ -99,11 +107,12 @@
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);
}
Index: source/API/SBMemoryRegionInfo.cpp
===================================================================
--- source/API/SBMemoryRegionInfo.cpp
+++ source/API/SBMemoryRegionInfo.cpp
@@ -20,11 +20,8 @@
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(MemoryRegionInfoUP &&lldb_object_up)
+ : m_opaque_ap(std::move(lldb_object_up)) {}
SBMemoryRegionInfo::SBMemoryRegionInfo(const SBMemoryRegionInfo &rhs)
: m_opaque_ap(new MemoryRegionInfo()) {
@@ -39,6 +36,12 @@
return *this;
}
+SBMemoryRegionInfo &SBMemoryRegionInfo::
+operator=(MemoryRegionInfoUP &&lldb_object_up) {
+ m_opaque_ap = std::move(lldb_object_up);
+ return *this;
+}
+
SBMemoryRegionInfo::~SBMemoryRegionInfo() {}
void SBMemoryRegionInfo::Clear() { m_opaque_ap->Clear(); }
Index: include/lldb/lldb-forward.h
===================================================================
--- include/lldb/lldb-forward.h
+++ include/lldb/lldb-forward.h
@@ -369,7 +369,6 @@
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;
Index: include/lldb/Target/Process.h
===================================================================
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -2081,7 +2081,7 @@
/// An error value.
//------------------------------------------------------------------
virtual Status
- GetMemoryRegions(std::vector<lldb::MemoryRegionInfoSP> ®ion_list);
+ GetMemoryRegions(std::vector<lldb::MemoryRegionInfoUP> ®ion_list);
virtual Status GetWatchpointSupportInfo(uint32_t &num) {
Status error;
Index: include/lldb/API/SBMemoryRegionInfoList.h
===================================================================
--- include/lldb/API/SBMemoryRegionInfoList.h
+++ include/lldb/API/SBMemoryRegionInfoList.h
@@ -26,13 +26,15 @@
~SBMemoryRegionInfoList();
- uint32_t GetSize() const;
+ size_t GetSize() const;
- bool GetMemoryRegionAtIndex(uint32_t idx, SBMemoryRegionInfo ®ion_info);
+ void Reserve(size_t capacity);
- void Append(lldb::SBMemoryRegionInfo ®ion);
+ bool GetMemoryRegionAtIndex(size_t idx, SBMemoryRegionInfo ®ion_info);
- void Append(lldb::SBMemoryRegionInfoList ®ion_list);
+ void Append(const lldb::SBMemoryRegionInfo ®ion);
+
+ void Append(const lldb::SBMemoryRegionInfoList ®ion_list);
void Clear();
Index: include/lldb/API/SBMemoryRegionInfo.h
===================================================================
--- include/lldb/API/SBMemoryRegionInfo.h
+++ include/lldb/API/SBMemoryRegionInfo.h
@@ -102,7 +102,9 @@
const lldb_private::MemoryRegionInfo &ref() const;
- SBMemoryRegionInfo(const lldb_private::MemoryRegionInfo *lldb_object_ptr);
+ SBMemoryRegionInfo(lldb::MemoryRegionInfoUP &&lldb_object_up);
+
+ SBMemoryRegionInfo &operator=(lldb::MemoryRegionInfoUP &&lldb_object_up);
lldb::MemoryRegionInfoUP m_opaque_ap;
};
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
