Skip to content

[lldb][progress][NFC] Add groundwork to keep track of progress reports #81026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[lldb][progress][NFC] Add groundwork to keep track of progress reports #81026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion lldb/include/lldb/Core/Progress.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "lldb/Utility/ConstString.h"
#include "lldb/lldb-types.h"
#include <atomic>
#include <map>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you need the sorted iteration property you probably want to use something more efficient instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shouldn't need a sorted map so I'll go with StringMap for the updated patch with the class that will handle the progress report map. 👍🏾

#include <mutex>
#include <optional>

Expand Down Expand Up @@ -55,6 +56,10 @@ namespace lldb_private {

class Progress {
public:
enum {
eProgressLinearReports,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please comment what each enum value means

eProgressCoalecseReports,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
eProgressCoalecseReports,
eProgressCoalesceReports,

};
Comment on lines +59 to +62
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a new setting for this? Something like:

(lldb) settings set progress-report [individual|grouped]

/// Construct a progress object that will report information.
///
/// The constructor will create a unique progress reporting object and
Expand All @@ -71,7 +76,8 @@ class Progress {
/// progress is to be reported only to specific debuggers.
Progress(std::string title, std::string details = {},
std::optional<uint64_t> total = std::nullopt,
lldb_private::Debugger *debugger = nullptr);
lldb_private::Debugger *debugger = nullptr,
bool type = Progress::eProgressLinearReports);

/// Destroy the progress object.
///
Expand Down Expand Up @@ -99,6 +105,10 @@ class Progress {
private:
void ReportProgress();
static std::atomic<uint64_t> g_id;
static std::atomic<uint64_t> g_refcount;
Copy link
Member

@medismailben medismailben Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By making the refcount static, it's shared across all the Progress instances, but IIUC, you want this refcount to be different for each progress category, right ? I actually I don't even think the Progress object needs to know about its refcount, since only the code that will handle the progress event (in Debugger::HandleProgressEvent) would need it.

/// Map that tracks each progress object and if we've seen its start and stop
/// events
static std::unordered_map<std::string, uint64_t> g_map;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this to be tied to the debugger instance instead of making it static here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We spoke about this a little offline but at the moment although Progress reports hold on to debugger instances, these instances are null by default and most Progress reports don't give a debugger instance in their instantiation so we wouldn't be able to have a debugger instance hold on to their own map of ongoing progress reports.

@clayborg Having a progress report holding on to a debugger instance is a good idea, but if we want to use a map to keep track of ongoing reports we need to know what will own the map? Do we want the map to be owned by a debugger instance or could the progress class hold on to its own map of ongoing reports? In the case of the former we would then want to enforce that progress reports have valid debugger instances and in the latter we could remove the debugger field from the reports altogether.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We spoke about this a little offline but at the moment although Progress reports hold on to debugger instances, these instances are null by default and most Progress reports don't give a debugger instance in their instantiation so we wouldn't be able to have a debugger instance hold on to their own map of ongoing progress reports.

@clayborg Having a progress report holding on to a debugger instance is a good idea, but if we want to use a map to keep track of ongoing reports we need to know what will own the map? Do we want the map to be owned by a debugger instance or could the progress class hold on to its own map of ongoing reports? In the case of the former we would then want to enforce that progress reports have valid debugger instances and in the latter we could remove the debugger field from the reports altogether.

The main question is if we want all progress reports to follow the currently selected mode (coalesce or individual) or if we want them to be able to pick which one they are. I would vote to have a global setting that controls if we deliver things, but would love to hear what others think.

Each Progress instance can have a debugger, but most don't, and those will get delivered to all debugger instances. So I would say that this map should live in each debugger object because Progress objects can have a debugger set, and if they do, they will only get delivered to that debugger.

So my suggestion here is to:

  • not change anything in Progress.h/cpp
  • move the progress categorization code into each debugger object
  • in PrivateReportProgress we check if anyone is listening to the new eBroadcastBitProgressByCategory bit, and then and only then do we do any of the category counting.

So the code in PrivateReportProgress will now look something like:

static void PrivateReportProgress(Debugger &debugger, uint64_t progress_id,
                                  std::string title, std::string details,
                                  uint64_t completed, uint64_t total,
                                  bool is_debugger_specific) {
  // Only deliver progress events if we have any progress listeners.
  if (debugger.GetBroadcaster().EventTypeHasListeners(Debugger::eBroadcastBitProgressByCategory))
    PrivateReportProgressByCategory(debugger, progress_id, is_debugger_specific, completed, total, is_debugger_specific);
  const uint32_t event_type = Debugger::eBroadcastBitProgress;
  if (!debugger.GetBroadcaster().EventTypeHasListeners(event_type))
    return;
  EventSP event_sp(new Event(
      event_type,
      new ProgressEventData(progress_id, std::move(title), std::move(details),
                            completed, total, is_debugger_specific)));
  debugger.GetBroadcaster().BroadcastEvent(event_sp);
}

Then we write a new PrivateReportProgressByCategory() function that will do all this book keeping and coordingate with the debugger which will own the category ref count map

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the debugger instance keep track of a map of progress reports falls in line with what I spoke about with Ismail offline and is the direction I'd vote to go in so that the bookkeeping isn't done in the progress objects (I'm sure this isn't impossible, but given that LLDB is hierarchical it would make more sense that debuggers have progress reports rather than progress reports looking after themselves).

So we still want to keep is_debugger_specific/ having a debugger pointer in a progress object, but do all the bookkeeping in all active debuggers and using that debugger pointer to only update the specific reports as needed if I'm understanding your approach correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Because we have to just in case the debugger pointer was set in the Progress, so we have to track it in each debugger just in case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of doing this at the debugger level? The way I imagined this to work is like the other subsystems (similar to what Greg described inline). The downside of doing the bookkeeping in the debugger, is that we'll have to duplicate all this work across debuggers for progress events that are not tied to a single debugger. I don't know how easy it would be to change that (if we have access to the debugger everywhere we report progress today). Either way I'd like to understand the motivation behind it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llvm::StringMap?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://llvm.org/doxygen/classllvm_1_1StringMap.html

/// The title of the progress activity.
std::string m_title;
std::string m_details;
Expand All @@ -117,6 +127,7 @@ class Progress {
/// to ensure that we don't send progress updates after progress has
/// completed.
bool m_complete = false;
bool m_type;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing, why would you store your enum value in a bool

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holdover from when I used a bool for this value before switching an enum when I put the patch up.

};

} // namespace lldb_private
Expand Down
21 changes: 18 additions & 3 deletions lldb/source/Core/Progress.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,34 @@
#include "lldb/Core/Progress.h"

#include "lldb/Core/Debugger.h"
#include "lldb/Utility/StreamString.h"

#include <optional>

using namespace lldb;
using namespace lldb_private;

std::atomic<uint64_t> Progress::g_id(0);
std::atomic<uint64_t> Progress::g_refcount(1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed. This value doesn't change and is always 1, so you can either make it a constexpr in the class description, or it isn't really needed at all actually.

std::unordered_map<std::string, uint64_t> Progress::g_map = {};
Comment on lines 18 to +20
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When something isn't just an integer, like the map here, we can run into issues with this in a multi-threaded environment. Why? These variables are part of the global C++ constructor / destructor chain. If any thread uses these after the main thread shuts down it can cause a crash. So for global objects we typically will create a pointer to one and leak it with a note. See lldb/source/Core/Debugger.cpp and look for g_debugger_list_mutex_ptr and other variables right next to them and then see Debugger::Initialize().

This map also need to be threadsafe, so it needs a mutex. Might be a good idea to create a class to contain these and use an accessor to provide access to it. The solution below will make something that is thread safe and also something that will survive the global destructor chain being called and letting other threads still be able to use Progress objects without crashing the process:

namespace {
class ProgressCategoryMap {
  std::mutex m_mutex;
  std::unordered_map<std::string, uint64_t> m_map;

  void Increment(std::string title) {
    std::lock_guard<std::mutex> lock(m_mutex);
    auto pair = g_map.emplace(title, 1); // No need to use g_refcount as it is just a constant 1
    // pair.first will be true if insertion happened.
    if (pair.first == false)
      ++pair.second; // Key already in map, increment the ref count.
  }
  void Decrement(std::string title) {
    std::lock_guard<std::mutex> lock(m_mutex);
    auto pos = g_map.find(title);
    if (pos == g_map.end())
      return;
    if (--pos->second == 0)
      g_map.erase(pos->second); // Decremented to zero
  }
};
// Now make a global accessor to get a threadsafe copy of ProgressCategoryMap
ProgressCategoryMap *GetProgressCategoryMap() {
  static std::once_flag g_once;
  static ProgressCategoryMap *g_category_map_ptr = nullptr;
  std::call_once(g_once, []{
    // NOTE: intentional leak to avoid issues with C++ destructor chain
    g_category_map_ptr = new ProgressCategoryMap();
  });
  return g_category_map_ptr;
}
} // anonymous namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better implementation of using a global map, but if we go with having the map being kept track of in each debugger object we might not need to have this implementation of a global map anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, might not be needed. Still needs to be threadsafe though if we do move it into the Debugger.h/.cpp

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you move it to the debugger instance, you don't need this anymore.


Progress::Progress(std::string title, std::string details,
std::optional<uint64_t> total,
lldb_private::Debugger *debugger)
lldb_private::Debugger *debugger, bool type)
: m_title(title), m_details(details), m_id(++g_id), m_completed(0),
m_total(Progress::kNonDeterministicTotal) {
m_total(Progress::kNonDeterministicTotal), m_type(type) {
if (total)
m_total = *total;

if (debugger)
m_debugger_id = debugger->GetID();
std::lock_guard<std::mutex> guard(m_mutex);

if (m_type == Progress::eProgressCoalecseReports) {
g_map.emplace(title, g_refcount);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a global refcount shouldn't be necessary here, we can just initialize the value to 1 and increment/decrement as needed.


if (g_map.at(title) >= 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be strictly greater than 1, otherwise you'll increment the refcount twice in the first progress report instantiation.

++g_map.at(title);
Comment on lines +35 to +38
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the threadsafe ProgressCategoryMap here:

GetProgressCategoryMap()->Increment(title);

}
ReportProgress();
}

Expand All @@ -38,6 +46,13 @@ Progress::~Progress() {
std::lock_guard<std::mutex> guard(m_mutex);
if (!m_completed)
m_completed = m_total;

if (m_type == Progress::eProgressCoalecseReports) {
--g_map.at(m_title);
if (g_map.at(m_title) == 0)
g_map.erase(m_title);
Comment on lines +51 to +53
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the threadsafe ProgressCategoryMap here:

GetProgressCategoryMap()->Decrement(title);

}

ReportProgress();
}

Expand Down