[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread via lldb-commits 

jeffreytan81 wrote:

Sounds good. Will keep the PR alive at least half day or one day before merging 
in future.

https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread David Blaikie via lldb-commits 


@@ -129,8 +129,19 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
 DWARFUnit &cu, llvm::function_ref callback) {
   uint64_t cu_offset = cu.GetOffset();
   bool found_entry_for_cu = false;
-  for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
-for (DebugNames::NameTableEntry nte: ni) {
+  for (const DebugNames::NameIndex &ni : *m_debug_names_up) {
+// Check if this name index contains an entry for the given CU.
+bool cu_matches = false;
+for (uint32_t i = 0; i < ni.getCUCount(); ++i) {
+  if (ni.getCUOffset(i) == cu_offset) {

dwblaikie wrote:

Oh, also, if you kept the result (more like a `llvm::find_if` as @bulbazord was 
suggesting, rather than my `llvm::none_of` here) of this search, you could save 
a small amount of time (no need to indirect through the index and reapply 
relocations to get the CU offset) by using `getCUInedx()` and comparing that to 
the index you would've found in this search - down on line 139/150

https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread David Blaikie via lldb-commits 


@@ -129,8 +129,19 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
 DWARFUnit &cu, llvm::function_ref callback) {
   uint64_t cu_offset = cu.GetOffset();
   bool found_entry_for_cu = false;
-  for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
-for (DebugNames::NameTableEntry nte: ni) {
+  for (const DebugNames::NameIndex &ni : *m_debug_names_up) {
+// Check if this name index contains an entry for the given CU.
+bool cu_matches = false;
+for (uint32_t i = 0; i < ni.getCUCount(); ++i) {
+  if (ni.getCUOffset(i) == cu_offset) {

dwblaikie wrote:

If NameIndex exposed the CUOffsets as a range (which seems pretty 
easy/reasonable for it to do - ah, because it requires potentially applying 
relocations it'd probably require a custom iterator - maybe a mapped iterator 
would be adequate & easy to do) then this could be written as:
```
if (llvm::none_of(ni.CUOffsets(), [&](uint64_t off) { return off == cu_offset; 
}))
  continue;
```

I /think/ `CUOffsets()` would look something roughly like this:
```
auto CUOffsets() const {
  assert(TU < Hdr.CompUnitCount);
  const unsigned SectionOffsetSize = dwarf::getDwarfOffsetByteSize(Hdr.Format);
  uint64_t Offset = CUsBase + SectionOffsetSize * CU;
  auto R = /* Guess we need some sort of generator to produce the values 
[CUsBase, CUsBase+SectionOffsetSize*Hdr.CompUnitCount) in SectionOffsetSize 
increments... 
some enhancement to llvm::seq that takes a stride size would be suitable */
  return llvm::map_range(llvm::seq(CUsBase, 
  [&](uint64_t Offset) {
return Section.AccelSection.getRelocatedValue(SectionOffsetSize, 
&Offset);
  });
}
```

https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread Alex Langford via lldb-commits 


@@ -129,8 +129,19 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
 DWARFUnit &cu, llvm::function_ref callback) {
   uint64_t cu_offset = cu.GetOffset();
   bool found_entry_for_cu = false;
-  for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
-for (DebugNames::NameTableEntry nte: ni) {
+  for (const DebugNames::NameIndex &ni : *m_debug_names_up) {
+// Check if this name index contains an entry for the given CU.
+bool cu_matches = false;
+for (uint32_t i = 0; i < ni.getCUCount(); ++i) {
+  if (ni.getCUOffset(i) == cu_offset) {
+cu_matches = true;
+break;
+  }
+}
+if (!cu_matches)
+  continue;

bulbazord wrote:

It'd be great if we could use some kind of `find_if` instead of manually 
walking the CU Offsets.

https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread Alex Langford via lldb-commits 

https://github.com/bulbazord edited 
https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread Alex Langford via lldb-commits 

https://github.com/bulbazord commented:

This patch looks fine to me in idea, but next time please leave your review up 
for longer so others have time to take a look as well.

https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread via lldb-commits 

llvmbot wrote:




@llvm/pr-subscribers-lldb

Author: None (jeffreytan81)


Changes

While using dwarf5 `.debug_names` in internal large targets, we noticed a 
performance issue (around 10 seconds delay) while `lldb-vscode` tries to show 
`scopes` for a compile unit. Profiling shows the bottleneck is inside 
`DebugNamesDWARFIndex::GetGlobalVariables` which linearly search all index 
entries belongs to a compile unit. 

This patch improves the performance by using the compile units list to filter 
first before checking index entries. This significantly improves the 
performance (drops from 10 seconds => under 1 second) in the split dwarf 
situation because each compile unit has its own named index. 




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


1 Files Affected:

- (modified) lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp 
(+13-2) 


``diff
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index 292ea2806c59dc7..4fc3866a3b608fd 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -129,8 +129,19 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
 DWARFUnit &cu, llvm::function_ref callback) {
   uint64_t cu_offset = cu.GetOffset();
   bool found_entry_for_cu = false;
-  for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
-for (DebugNames::NameTableEntry nte: ni) {
+  for (const DebugNames::NameIndex &ni : *m_debug_names_up) {
+// Check if this name index contains an entry for the given CU.
+bool cu_matches = false;
+for (uint32_t i = 0; i < ni.getCUCount(); ++i) {
+  if (ni.getCUOffset(i) == cu_offset) {
+cu_matches = true;
+break;
+  }
+}
+if (!cu_matches)
+  continue;
+
+for (DebugNames::NameTableEntry nte : ni) {
   uint64_t entry_offset = nte.getEntryOffset();
   llvm::Expected entry_or = ni.getEntry(&entry_offset);
   for (; entry_or; entry_or = ni.getEntry(&entry_offset)) {

``




https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread via lldb-commits 

https://github.com/jeffreytan81 closed 
https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread via lldb-commits 

https://github.com/jeffreytan81 ready_for_review 
https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread via lldb-commits 

https://github.com/jeffreytan81 updated 
https://github.com/llvm/llvm-project/pull/70231

>From c8d9a1f7387e3e4944d92ccb33699b6a9d0dcf89 Mon Sep 17 00:00:00 2001
From: jeffreytan81 
Date: Wed, 25 Oct 2023 10:21:06 -0700
Subject: [PATCH 1/2] Improve debug names index fetching global variables
 performance

---
 .../Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index 292ea2806c59dc7..f94491b29bdbe14 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -130,6 +130,17 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
   uint64_t cu_offset = cu.GetOffset();
   bool found_entry_for_cu = false;
   for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
+// Check if this name index contains an entry for the given CU.
+bool has_match_cu = false;
+for (uint32_t i = 0;i < ni.getCUCount();++i) {
+  if (ni.getCUOffset(i) == cu_offset) {
+has_match_cu = true;
+break;
+  }
+}
+if (!has_match_cu)
+  continue;
+
 for (DebugNames::NameTableEntry nte: ni) {
   uint64_t entry_offset = nte.getEntryOffset();
   llvm::Expected entry_or = ni.getEntry(&entry_offset);

>From 19b7055e813471728f95ca42efcd4802261f2ea3 Mon Sep 17 00:00:00 2001
From: jeffreytan81 
Date: Wed, 25 Oct 2023 11:15:14 -0700
Subject: [PATCH 2/2] Address formatter/review feedback

---
 .../SymbolFile/DWARF/DebugNamesDWARFIndex.cpp| 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index f94491b29bdbe14..4fc3866a3b608fd 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -129,19 +129,19 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
 DWARFUnit &cu, llvm::function_ref callback) {
   uint64_t cu_offset = cu.GetOffset();
   bool found_entry_for_cu = false;
-  for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
+  for (const DebugNames::NameIndex &ni : *m_debug_names_up) {
 // Check if this name index contains an entry for the given CU.
-bool has_match_cu = false;
-for (uint32_t i = 0;i < ni.getCUCount();++i) {
+bool cu_matches = false;
+for (uint32_t i = 0; i < ni.getCUCount(); ++i) {
   if (ni.getCUOffset(i) == cu_offset) {
-has_match_cu = true;
+cu_matches = true;
 break;
   }
 }
-if (!has_match_cu)
+if (!cu_matches)
   continue;
 
-for (DebugNames::NameTableEntry nte: ni) {
+for (DebugNames::NameTableEntry nte : ni) {
   uint64_t entry_offset = nte.getEntryOffset();
   llvm::Expected entry_or = ni.getEntry(&entry_offset);
   for (; entry_or; entry_or = ni.getEntry(&entry_offset)) {

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread via lldb-commits 

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff 8efd6799f006000a5e3aacbb403f262db37290b6 
c8d9a1f7387e3e4944d92ccb33699b6a9d0dcf89 -- 
lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
``





View the diff from clang-format here.


``diff
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index f94491b29bdb..d5ff2f0da9ac 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -132,7 +132,7 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
   for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
 // Check if this name index contains an entry for the given CU.
 bool has_match_cu = false;
-for (uint32_t i = 0;i < ni.getCUCount();++i) {
+for (uint32_t i = 0; i < ni.getCUCount(); ++i) {
   if (ni.getCUOffset(i) == cu_offset) {
 has_match_cu = true;
 break;

``




https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread Greg Clayton via lldb-commits 

https://github.com/clayborg edited 
https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread Greg Clayton via lldb-commits 

https://github.com/clayborg approved this pull request.

Looks good, just rename the "has_match_cu" to "cu_matches" and this is good to 
go.

https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread Greg Clayton via lldb-commits 


@@ -130,6 +130,17 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
   uint64_t cu_offset = cu.GetOffset();
   bool found_entry_for_cu = false;
   for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
+// Check if this name index contains an entry for the given CU.
+bool has_match_cu = false;

clayborg wrote:

I would rename this maybe to "cu_matches".

https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)

2023-10-25 Thread via lldb-commits 

https://github.com/jeffreytan81 created 
https://github.com/llvm/llvm-project/pull/70231

While using dwarf5 `.debug_names` in internal large targets, we noticed a 
performance issue (around 10 seconds delay) while `lldb-vscode` tries to show 
`scopes` for a compile unit. Profiling shows the bottleneck is inside 
`DebugNamesDWARFIndex::GetGlobalVariables` which linearly search all index 
entries belongs to a compile unit. 

This patch improves the performance by using the compile units list to filter 
first before checking index entries. This significantly improves the 
performance (drops from 10 seconds => under 1 second) in the split dwarf 
situation because each compile unit has its own named index. 




>From c8d9a1f7387e3e4944d92ccb33699b6a9d0dcf89 Mon Sep 17 00:00:00 2001
From: jeffreytan81 
Date: Wed, 25 Oct 2023 10:21:06 -0700
Subject: [PATCH] Improve debug names index fetching global variables
 performance

---
 .../Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
index 292ea2806c59dc7..f94491b29bdbe14 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp
@@ -130,6 +130,17 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
   uint64_t cu_offset = cu.GetOffset();
   bool found_entry_for_cu = false;
   for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
+// Check if this name index contains an entry for the given CU.
+bool has_match_cu = false;
+for (uint32_t i = 0;i < ni.getCUCount();++i) {
+  if (ni.getCUOffset(i) == cu_offset) {
+has_match_cu = true;
+break;
+  }
+}
+if (!has_match_cu)
+  continue;
+
 for (DebugNames::NameTableEntry nte: ni) {
   uint64_t entry_offset = nte.getEntryOffset();
   llvm::Expected entry_or = ni.getEntry(&entry_offset);

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits