[lldb][progress] Add progress manager class #81319

chelcassanova · 2024-02-09T21:11:45Z

Per discussions from #81026, it was decided that having a class that manages a map of progress reports would be beneficial in order to categorize them. This class is a part of the overall Progress class and utilizes a map that keeps a count of how many times a progress report category has been sent. This would be used with the new debugger broadcast bit added in #81169 so that a user listening with that bit will receive grouped progress reports.

llvmbot · 2024-02-09T21:12:18Z

@llvm/pr-subscribers-lldb

Author: Chelsea Cassanova (chelcassanova)

Changes

Per discussions from #81026, it was decided that having a class that manages a map of progress reports would be beneficial in order to categorize them. This class is a part of the overall Progress class and utilizes a map that keeps a count of how many times a progress report category has been sent. This would be used with the new debugger broadcast bit added in #81169 so that a user listening with that bit will receive grouped progress reports.

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

2 Files Affected:

(modified) lldb/include/lldb/Core/Progress.h (+27)
(modified) lldb/source/Core/Progress.cpp (+44)

diff --git a/lldb/include/lldb/Core/Progress.h b/lldb/include/lldb/Core/Progress.h
index 5d882910246054..7ac6881857445f 100644
--- a/lldb/include/lldb/Core/Progress.h
+++ b/lldb/include/lldb/Core/Progress.h
@@ -11,6 +11,7 @@
 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-types.h"
+#include "llvm/ADT/StringMap.h"
 #include <atomic>
 #include <mutex>
 #include <optional>
@@ -119,6 +120,32 @@ class Progress {
   bool m_complete = false;
 };
 
+/// A class used to group progress reports by category. This is done by using a
+/// map that maintains a refcount of each category of progress reports that have
+/// come in. Keeping track of progress reports this way will be done if a
+/// debugger is listening to the eBroadcastBitProgressByCategory broadcast bit.
+class ProgressManager {
+public:
+  ProgressManager();
+  ~ProgressManager();
+
+  static void Initialize();
+  static void Terminate();
+  // Control the refcount of the progress report category as needed
+  void Increment(std::string);
+  void Decrement(std::string);
+
+  // Public accessor for the class instance
+  static ProgressManager &Instance();
+
+private:
+  // Manage the class instance internally using a std::optional
+  static std::optional<ProgressManager> &InstanceImpl();
+
+  llvm::StringMap<uint64_t> m_progress_map;
+  std::mutex m_progress_map_mutex;
+};
+
 } // namespace lldb_private
 
 #endif // LLDB_CORE_PROGRESS_H
diff --git a/lldb/source/Core/Progress.cpp b/lldb/source/Core/Progress.cpp
index 732efbc342b450..4eb1ea7b8cad4d 100644
--- a/lldb/source/Core/Progress.cpp
+++ b/lldb/source/Core/Progress.cpp
@@ -66,3 +66,47 @@ void Progress::ReportProgress() {
                              m_debugger_id);
   }
 }
+
+void ProgressManager::Initialize() {
+  lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+  InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+  lldbassert(InstanceImpl() &&
+             "A progress report manager has already been terminated.");
+  InstanceImpl().reset();
+}
+
+std::optional<ProgressManager> &ProgressManager::InstanceImpl() {
+  static std::optional<ProgressManager> g_progress_manager;
+  return g_progress_manager;
+}
+
+ProgressManager::ProgressManager() : m_progress_map() {}
+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager &ProgressManager::Instance() { return *InstanceImpl(); }
+
+void ProgressManager::Increment(std::string title) {
+  std::lock_guard<std::mutex> lock(m_progress_map_mutex);
+  auto pair = m_progress_map.insert(std::pair(title, 1));
+
+  // If pair.first is not empty after insertion it means that that
+  // category was entered for the first time and should not be incremented
+  if (!pair.first->first().empty())
+    ++pair.first->second;
+}
+
+void ProgressManager::Decrement(std::string title) {
+  std::lock_guard<std::mutex> lock(m_progress_map_mutex);
+  auto pos = m_progress_map.find(title);
+
+  if (pos == m_progress_map.end())
+    return;
+
+  // Remove the category from the map if the refcount reaches 0
+  if (--pos->second == 0)
+    m_progress_map.erase(title);
+}

clayborg · 2024-02-09T22:34:37Z

lldb/include/lldb/Core/Progress.h

+  // Manage the class instance internally using a std::optional
+  static std::optional<ProgressManager> &InstanceImpl();
+
+  llvm::StringMap<uint64_t> m_progress_map;


maybe m_category_map?

We could do m_progress_category_map or m_progress_report_map to better convey that we're using this to keep track of progress reports by category.

clayborg · 2024-02-09T22:35:35Z

lldb/include/lldb/Core/Progress.h

+  void Increment(std::string);
+  void Decrement(std::string);


void Increment(std::string category); void Decrement(std::string category);

clayborg · 2024-02-09T22:37:59Z

lldb/source/Core/Progress.cpp

+void ProgressManager::Initialize() {
+  lldbassert(!InstanceImpl() && "A progress report manager already exists.");
+  InstanceImpl().emplace();
+}
+
+void ProgressManager::Terminate() {
+  lldbassert(InstanceImpl() &&
+             "A progress report manager has already been terminated.");
+  InstanceImpl().reset();
+}


We can avoid the initialize/terminate if we just want to create this object on the first call to ProgressManager::InstanceImpl(). More comments below as to why

clayborg · 2024-02-09T22:40:08Z

lldb/source/Core/Progress.cpp

+  static std::optional<ProgressManager> g_progress_manager;
+  return g_progress_manager;


The global destructor chain will still cause this to be destroyed when the main thread exits. Which is ok if we return std::optional<ProgressManager> from ProgressManager::Instance(). More below in ProgressManager &ProgressManager::Instance() comments.

clayborg · 2024-02-09T22:44:43Z

lldb/source/Core/Progress.cpp

+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager &ProgressManager::Instance() { return *InstanceImpl(); }


If we want to use a static variable in std::optional<ProgressManager> &ProgressManager::InstanceImpl(), then we need to return a std::optional<ProgressManager> here and people need to check it and not use it if the optional isn't valid. If we always want to return a "ProgressManager &" here, then we need InstanceImpl() to look like:

ProgressManager &ProgressManager::InstanceImpl() { static std::once g_once_flag; static ProgressManager *g_progress_manager = nullptr; std::call_once(std::call_once(g_once_flag, { // NOTE: known leak to avoid global destructor chain issues. g_progress_manager = new ProgressManager(); }); return *g_progress_manager; }

And then we don't need ProgressManager::Initialize() or ProgressManager::Terminate().

The problem is when the process exits, the main thread exists and calls the C++ global destructor chain and if any threads are still running and call any of these functions that require the global instance of ProgressManager, it can crash the process.

We have other subsystems that work exactly like what Chelsea did here, such as the Diagnostics and the FileSystem. They rely on the requirement that you need to call ::Initialize before doing anything with it. It's true that this will crash if you forget to Initialize/Terminate but that's true for the Debugger too. This is an establishes pattern and has the advantage that it can be tested in unittest by initializing and terminating the subsystem between tests.

We have other subsystems that work exactly like what Chelsea did here, such as the Diagnostics and the FileSystem. They rely on the requirement that you need to call ::Initialize before doing anything with it. It's true that this will crash if you forget to Initialize/Terminate but that's true for the Debugger too. This is an establishes pattern and has the advantage that it can be tested in unittest by initializing and terminating the subsystem between tests.

This is fine as long as you always check the optional then and we aren't doing that. So this can and will crash LLDB if extra threads call these APIs which is never good. So the API, in order to be safe, should be:

std::optional<ProgressManager &> ProgressManager::InstanceImpl()

And then every user needs to check this optional. Otherwise, this:

ProgressManager &ProgressManager::Instance() { return *InstanceImpl(); }

Will crash if Terminate has been called right? the std:optional::operator *docs say "The behavior is undefined if *this does not contain a value."

This can cause shut down crashes in LLDB where extra threads call a progress API.

Another reason for using the std::once is if no one tries to listen to the eBroadcastBitProgressCategory, then nothing will need to be created. If not we are always making an instance. Granted this object isn't very expensive to create, but it is extra work that might not be needed.

It is fine to start with this and switch over eventually if we run into problems. Not sure what percentage of people will listen to the category progresses vs the individual ones. So it will affect only the people using the category ones.

But wether you leak it or not it won't affect the program since it is exiting already when this gets cleaned up if it is left as is above. So up to you guys.

So we'll keep the current functionality (using Initialize/Terminate, and having the user check the optional instead of doing it in the class) if I'm understanding correctly?

Works for me as long as all clients check the optional and use it correctly. It allows for multi-thread safety so that is good. Not sure if std::optional is actually threadsafe though, but the timing required to make this fail would be pretty low percentage. If it were me I would make it bullet proof as just leak the map as when we tear down the process, any allocated memory will just go poof anyway and it will actually speed up the process teardown process as no work will need to be done to clear this map, though it should really be empty anyway.

Given that this patch follows a pattern established by the other subsystems, and those subsystems don't currently show any major issues in production with their initialization and termination lifecycle I think we can follow the current subsystem behaviour for this patch and discussions about the subsystems themselves can take place in a separate discussion.

clayborg · 2024-02-09T22:48:36Z

lldb/source/Core/Progress.cpp

+
+  // If pair.first is not empty after insertion it means that that
+  // category was entered for the first time and should not be incremented
+  if (!pair.first->first().empty())


This returns a std::pair<iterator, bool> where the bool designates if the instertion took place. So this should be:

if (!pair.second)

Per discussions from llvm#81026, it was decided that having a class that manages a map of progress reports would be beneficial in order to categorize them. This class is a part of the overall `Progress` class and utilizes a map that keeps a count of how many times a progress report category has been sent. This would be used with the new debugger broadcast bit added in llvm#81169 so that a user listening with that bit will receive grouped progress reports.

clayborg

Looks good!

JDevlieghere

LGTM with the Initialize/Terminate removed as they're not needed anymore.

JDevlieghere · 2024-02-14T01:59:06Z

lldb/include/lldb/Core/Progress.h

+  static void Initialize();
+  static void Terminate();


These are not implemented (and no longer needed)?

Correct, I'll remove these.

adrian-prantl · 2024-02-14T17:42:25Z

lldb/include/lldb/Core/Progress.h

+
+  static void Initialize();
+  static void Terminate();
+  // Control the refcount of the progress report category as needed


adrian-prantl · 2024-02-14T17:42:39Z

lldb/include/lldb/Core/Progress.h

+  void Increment(std::string category);
+  void Decrement(std::string category);
+
+  // Public accessor for the class instance


/// and . at the end of the sentence.

Just remove this, they don't add anything and we're slowly removing them.

adrian-prantl · 2024-02-14T17:42:53Z

lldb/include/lldb/Core/Progress.h

+  static ProgressManager &Instance();
+
+private:
+  // Manage the class instance internally using a std::optional


adrian-prantl · 2024-02-14T17:43:12Z

lldb/source/Core/Progress.cpp

+  auto pair = m_progress_category_map.insert(std::pair(title, 1));
+
+  // If pair.first is not empty after insertion it means that that
+  // category was entered for the first time and should not be incremented


. at the end of sentence

adrian-prantl · 2024-02-14T17:43:17Z

lldb/source/Core/Progress.cpp

+  if (pos == m_progress_category_map.end())
+    return;
+
+  // Remove the category from the map if the refcount reaches 0


adrian-prantl · 2024-02-14T17:44:10Z

lldb/source/Core/Progress.cpp

+    return;
+
+  // Remove the category from the map if the refcount reaches 0
+  if (--pos->second == 0)


What guards against pos->second being 0? Should that be an assertion at least?

I added a check to return early if pos->second == 0.

In order to prevent potential crashes when LLDB is terminated, this commit instead creates the instance for the progress manager in a `call_once` within its initializer.

JDevlieghere · 2024-02-15T03:26:36Z

lldb/source/Core/Progress.cpp

+  // If pair.first is not empty after insertion it means that that
+  // category was entered for the first time and should not be incremented.
+  if (!pair.second)
+    ++pair.first->second;


Any reason this should be a pre-increment? pair.first returns an iterator so without checking the C++ reference, I don't know if this is incrementing the or the value. Thinking about it some more, you can simplify this a lot:

std::lock_guard<std::mutex> lock(m_progress_map_mutex); m_progress_category_map[title]++;

operator[] will return a default constructed value (i.e. 0) and then the increment will set it to one.

Nice, no need to check the status of the map insertion in the first place.

JDevlieghere · 2024-02-15T03:31:26Z

lldb/source/Core/Progress.cpp

+  std::lock_guard<std::mutex> lock(m_progress_map_mutex);
+  auto pos = m_progress_category_map.find(title);
+
+  if (pos == m_progress_category_map.end() || pos->second == 0)


If pos->second is 0, don't we still need to remove it? How about unconditionally removing it when it's <= 1 and otherwise decrementing it?

Increment() now uses the operator[] which removes the need that the insertion in the map was of a new category. Decrement() removes the category if its refcount is <= 1, otherwise it will then decrement it.

clayborg

Just remove static ProgressManager &InstanceImpl(); and inline the code into static ProgressManager &Instance(); and this is good to go.

clayborg · 2024-02-14T19:32:24Z

lldb/include/lldb/Core/Progress.h

+  static ProgressManager &Instance();
+
+private:
+  // Manage the class instance internally using a std::optional


Is this comment out of date? I don't think we are using std::optional<T> anymore

I should've changed this comment in a more recent push that removes the reference to the std::optional.

clayborg · 2024-02-14T19:33:26Z

lldb/source/Core/Progress.cpp

+ProgressManager &ProgressManager::InstanceImpl() {
+  static std::once_flag g_once_flag;
+  static ProgressManager *g_progress_manager = nullptr;
+  std::call_once(g_once_flag, []() {
+    // NOTE: known leak to avoid global destructor chain issues.
+    g_progress_manager = new ProgressManager();
+  });
+  return *g_progress_manager;
+}


We probably don't need this function and we can just inline the contents into the static ProgressManager &ProgressManager::Instance() right?

I was actually going to ask if Instance() was still necessary given that all functionality was in InstanceImpl() (or if InstanceImpl() now needed to be public or something along those lines). If we're going to keep the implementation of the instance accessor this way then to my understanding we can inline Instance().

clayborg · 2024-02-15T19:30:18Z

lldb/source/Core/Progress.cpp

+
+ProgressManager::~ProgressManager() {}
+
+ProgressManager &ProgressManager::Instance() { return InstanceImpl(); }


Remove ProgressManager &ProgressManager::InstanceImpl() and inline the code from that function in this function.

Removes `InstanceImpl()` and inlines its code into `Instance()`.

Per discussions from llvm#81026, it was decided that having a class that manages a map of progress reports would be beneficial in order to categorize them. This class is a part of the overall `Progress` class and utilizes a map that keeps a count of how many times a progress report category has been sent. This would be used with the new debugger broadcast bit added in llvm#81169 so that a user listening with that bit will receive grouped progress reports. (cherry picked from commit 327d2f8)

This commit adds the functionality to broadcast events using the `Debugger::eBroadcastProgressCategory` bit (llvm#81169) by keeping track of these reports with the `ProgressManager` class (llvm#81319). The new bit is used in such a way that it will only broadcast the initial and final progress reports for specific progress categories that are managed by the progress manager. This commit also adds a new test to the progress report unit test that checks that only the initial/final reports are broadcasted when using the new bit.

This commit adds the functionality to broadcast events using the `Debugger::eBroadcastProgressCategory` bit (#81169) by keeping track of these reports with the `ProgressManager` class (#81319). The new bit is used in such a way that it will only broadcast the initial and final progress reports for specific progress categories that are managed by the progress manager. This commit also adds a new test to the progress report unit test that checks that only the initial/final reports are broadcasted when using the new bit.

…#83069) This commit adds the functionality to broadcast events using the `Debugger::eBroadcastProgressCategory` bit (llvm#81169) by keeping track of these reports with the `ProgressManager` class (llvm#81319). The new bit is used in such a way that it will only broadcast the initial and final progress reports for specific progress categories that are managed by the progress manager. This commit also adds a new test to the progress report unit test that checks that only the initial/final reports are broadcasted when using the new bit. rdar://120788399 (cherry picked from commit 137ed17)

…#83069) (#8317) This commit adds the functionality to broadcast events using the `Debugger::eBroadcastProgressCategory` bit (llvm#81169) by keeping track of these reports with the `ProgressManager` class (llvm#81319). The new bit is used in such a way that it will only broadcast the initial and final progress reports for specific progress categories that are managed by the progress manager. This commit also adds a new test to the progress report unit test that checks that only the initial/final reports are broadcasted when using the new bit. rdar://120788399 (cherry picked from commit 137ed17)

chelcassanova requested review from adrian-prantl, clayborg and medismailben February 9, 2024 21:11

chelcassanova requested a review from JDevlieghere as a code owner February 9, 2024 21:11

llvmbot added the lldb label Feb 9, 2024

clayborg requested changes Feb 9, 2024

View reviewed changes

chelcassanova force-pushed the progress-manager branch from 27d8539 to f5ef078 Compare February 10, 2024 01:22

clayborg approved these changes Feb 14, 2024

View reviewed changes

JDevlieghere approved these changes Feb 14, 2024

View reviewed changes

adrian-prantl reviewed Feb 14, 2024

View reviewed changes

Use call_once in InstanceImpl(), leak progress manager object

5a04596

In order to prevent potential crashes when LLDB is terminated, this commit instead creates the instance for the progress manager in a `call_once` within its initializer.

chelcassanova force-pushed the progress-manager branch from 67886f4 to 5a04596 Compare February 14, 2024 20:31

JDevlieghere reviewed Feb 15, 2024

View reviewed changes

Simpify Increment() and Decrement()

d341ce9

Increment() now uses the operator[] which removes the need that the insertion in the map was of a new category. Decrement() removes the category if its refcount is <= 1, otherwise it will then decrement it.

JDevlieghere approved these changes Feb 15, 2024

View reviewed changes

clayborg reviewed Feb 15, 2024

View reviewed changes

Remove InstanceImpl()

da98a76

Removes `InstanceImpl()` and inlines its code into `Instance()`.

chelcassanova merged commit 327d2f8 into llvm:main Feb 15, 2024

chelcassanova mentioned this pull request Feb 26, 2024

[lldb][progress] Hook up new broadcast bit and progress manager #83069

Merged

chelcassanova mentioned this pull request Mar 1, 2024

[lldb][progress] Hook up new broadcast bit and progress manager (#83069) swiftlang/llvm-project#8317

Merged

		static std::optional<ProgressManager> g_progress_manager;
		return g_progress_manager;


		ProgressManager::~ProgressManager() {}

		ProgressManager &ProgressManager::Instance() { return *InstanceImpl(); }

[lldb][progress] Add progress manager class #81319

[lldb][progress] Add progress manager class #81319

Uh oh!

Conversation

chelcassanova commented Feb 9, 2024

Uh oh!

llvmbot commented Feb 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDevlieghere Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JDevlieghere Feb 9, 2024 •

edited

Loading