[lldb] Create a default rate limit constant in Progress (NFC)

[JDevlieghere]

In #133211, Greg suggested making the rate limit configurable through a setting. Although adding the setting is easy, the two places where we currently use rate limiting aren't tied to a particular debugger. Although it'd be possible to hook up, given how few progress events currently implement rate limiting, I don't think it's worth threading this through, if that's even possible.

I still think it's a good idea to be consistent and make it easy to pick the same rate limiting value, so I've moved it into a constant in the Progress class.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

In #133211, Greg suggested making the rate limit configurable through a setting. Although adding the setting is easy, the two places where we currently use rate limiting aren't tied to a particular debugger.

I still think it's a good idea to be consistent and make it easy to pick the same rate limiting value, so I've moved it into a constant in the Progress class.

---

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

3 Files Affected:

• (modified) lldb/include/lldb/Core/Progress.h (+4)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp (+1-1)
• (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp (+1-2)

diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index 3003568e8946b..104a8c0feb80a 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -115,6 +115,10 @@ class Progress {
   /// Used to indicate a non-deterministic progress report
   static constexpr uint64_t kNonDeterministicTotal = UINT64_MAX;
 
+  /// The default rate limit for high frequency progress reports.
+  static constexpr std::chrono::milliseconds kDefaultRateLimit =
+      std::chrono::milliseconds(20);
+
 private:
   void ReportProgress();
   static std::atomic<uint64_t> g_id;
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
index 6f2c45e74132c..f6304a7a4b9ba 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -81,7 +81,7 @@ void ManualDWARFIndex::Index() {
   const uint64_t total_progress = units_to_index.size() * 2 + 8;
   Progress progress("Manually indexing DWARF", module_desc.GetData(),
                     total_progress, /*debugger=*/nullptr,
-                    /*minimum_report_time=*/std::chrono::milliseconds(20));
+                    Progress::kDefaultRateLimit);
 
   // Share one thread pool across operations to avoid the overhead of
   // recreating the threads.
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
index e346d588a449f..8b72c96dc6f33 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
@@ -723,8 +723,7 @@ void SymbolFileDWARFDebugMap::ForEachSymbolFile(
     std::function<IterationAction(SymbolFileDWARF &)> closure) {
   const size_t num_oso_idxs = m_compile_unit_infos.size();
   Progress progress(std::move(description), "", num_oso_idxs,
-                    /*debugger=*/nullptr,
-                    /*minimum_report_time=*/std::chrono::milliseconds(20));
+                    /*debugger=*/nullptr, Progress::kDefaultRateLimit);
   for (uint32_t oso_idx = 0; oso_idx < num_oso_idxs; ++oso_idx) {
     if (SymbolFileDWARF *oso_dwarf = GetSymbolFileByOSOIndex(oso_idx)) {
       progress.Increment(oso_idx, oso_dwarf->GetObjectFile()

[labath]

I don't think this needs to be a setting as the goal here is to weed out truly egregious report frequencies (O(thousands) per second). I think the rest would be better handled at the receiving side.

I guess making a constant for that is fine, though the name is somewhat unfortunate as this isn't really the default (the default is no rate limiting). It sort of makes sense if you tilt your head the right way, but it's not ideal that the tilting is required. How about kDefaultHighFrequencyReportTime (it's a mouthful, but I guess we won't use it often)?

Two other random ideas:

• maybe we could actually make this the default (for all progress reports)? The risk is that there's some reports get lost (e.g. if we send three reports, and the first two are finished very quickly, then the last two could be dropped, and the progress would show the first step as taking a long time, even though it has actually finished already), but maybe we're fine with that?
• if we go with the above, and given what I've said the first paragraph, maybe this doesn't actually need to be settable, from anywhere?

[JDevlieghere]

I don't think this needs to be a setting as the goal here is to weed out truly egregious report frequencies (O(thousands) per second). I think the rest would be better handled at the receiving side.

👍

I guess making a constant for that is fine, though the name is somewhat unfortunate as this isn't really the default (the default is no rate limiting). It sort of makes sense if you tilt your head the right way, but it's not ideal that the tilting is required. How about kDefaultHighFrequencyReportTime (it's a mouthful, but I guess we won't use it often)?

Works for me.

Two other random ideas:

• maybe we could actually make this the default (for all progress reports)? The risk is that there's some reports get lost (e.g. if we send three reports, and the first two are finished very quickly, then the last two could be dropped, and the progress would show the first step as taking a long time, even though it has actually finished already), but maybe we're fine with that?
• if we go with the above, and given what I've said the first paragraph, maybe this doesn't actually need to be settable, from anywhere?

I considered this too and decided against it for the same reason. Misattributing where time is spent really undermines the value of the progress reports. For now I believe it's best to rely on our expertise to categorize progress reports that should be rate limited.