-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[LLDB][Data Formatters] Calculate average and total time for summary providers within lldb #102708
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.
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
Conversation
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesThis PR adds a statistics provider cache, which allows an individual target to keep a rolling tally of it's total time and number of invocations for a given summary provider. This information is then available in statistics dump to help slow summary providers, and gleam more into insight into LLDB's time use. Full diff: https://github.com/llvm/llvm-project/pull/102708.diff 8 Files Affected:
diff --git a/lldb/include/lldb/Target/Statistics.h b/lldb/include/lldb/Target/Statistics.h
index 35bd7f8a66e055..06ca5c7923f747 100644
--- a/lldb/include/lldb/Target/Statistics.h
+++ b/lldb/include/lldb/Target/Statistics.h
@@ -16,6 +16,7 @@
#include "llvm/Support/JSON.h"
#include <atomic>
#include <chrono>
+#include <mutex>
#include <optional>
#include <ratio>
#include <string>
@@ -25,6 +26,7 @@ namespace lldb_private {
using StatsClock = std::chrono::high_resolution_clock;
using StatsTimepoint = std::chrono::time_point<StatsClock>;
+using Duration = std::chrono::duration<double>;
class StatsDuration {
public:
@@ -174,6 +176,83 @@ struct StatisticsOptions {
std::optional<bool> m_include_transcript;
};
+/// A class that represents statistics about a TypeSummaryProviders invocations
+class SummaryStatistics {
+public:
+ SummaryStatistics() = default;
+ SummaryStatistics(lldb_private::ConstString name) :
+ m_total_time(), m_name(name), m_summary_count(0) {}
+
+ SummaryStatistics(const SummaryStatistics &&rhs)
+ : m_total_time(), m_name(rhs.m_name), m_summary_count(rhs.m_summary_count.load(std::memory_order_relaxed)) {}
+
+ lldb_private::ConstString GetName() const { return m_name; };
+ double GetAverageTime() const {
+ return m_total_time.get().count() / m_summary_count.load(std::memory_order_relaxed);
+ }
+
+ double GetTotalTime() const {
+ return m_total_time.get().count();
+ }
+
+ uint64_t GetSummaryCount() const {
+ return m_summary_count.load(std::memory_order_relaxed);
+ }
+
+ StatsDuration& GetDurationReference() {
+ return m_total_time;
+ }
+
+ llvm::json::Value ToJSON() const;
+
+ // Basic RAII class to increment the summary count when the call is complete.
+ // In the future this can be extended to collect information about the
+ // elapsed time for a single request.
+ class SummaryInvocation {
+ public:
+ SummaryInvocation(SummaryStatistics &summary) : m_summary(summary) {}
+ ~SummaryInvocation() {
+ m_summary.OnInvoked();
+ }
+ private:
+ SummaryStatistics &m_summary;
+ };
+
+private:
+ /// Called when Summary Invocation is destructed.
+ void OnInvoked() {
+ m_summary_count.fetch_add(1, std::memory_order_relaxed);
+ }
+
+ lldb_private::StatsDuration m_total_time;
+ lldb_private::ConstString m_name;
+ std::atomic<uint64_t> m_summary_count;
+};
+
+/// A class that wraps a std::map of SummaryStatistics objects behind a mutex.
+class SummaryStatisticsCache {
+public:
+ SummaryStatisticsCache() = default;
+ /// Get the SummaryStatistics object for a given provider name, or insert
+ /// if statistics for that provider is not in the map.
+ lldb_private::SummaryStatistics &GetSummaryStatisticsForProviderName(lldb_private::ConstString summary_provider_name) {
+ m_map_mutex.lock();
+ if (m_summary_stats_map.count(summary_provider_name) == 0) {
+ m_summary_stats_map.emplace(summary_provider_name, SummaryStatistics(summary_provider_name));
+ }
+
+ SummaryStatistics &summary_stats = m_summary_stats_map.at(summary_provider_name);
+ m_map_mutex.unlock();
+ return summary_stats;
+ }
+
+ llvm::json::Value ToJSON();
+
+private:
+ std::map<lldb_private::ConstString, lldb_private::SummaryStatistics> m_summary_stats_map;
+ std::mutex m_map_mutex;
+};
+
/// A class that represents statistics for a since lldb_private::Target.
class TargetStats {
public:
@@ -198,6 +277,7 @@ class TargetStats {
StatsSuccessFail m_frame_var{"frameVariable"};
std::vector<intptr_t> m_module_identifiers;
uint32_t m_source_map_deduce_count = 0;
+ SummaryStatisticsCache m_summary_stats_cache;
void CollectStats(Target &target);
};
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 119dff4d498199..ae1ea43c01003b 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -258,6 +258,7 @@ class TargetProperties : public Properties {
bool GetDebugUtilityExpression() const;
+
private:
std::optional<bool>
GetExperimentalPropertyValue(size_t prop_idx,
@@ -1221,6 +1222,9 @@ class Target : public std::enable_shared_from_this<Target>,
void ClearAllLoadedSections();
+ lldb_private::SummaryStatistics& GetSummaryStatisticsForProvider(lldb_private::ConstString summary_provider_name);
+ lldb_private::SummaryStatisticsCache& GetSummaryStatisticsCache();
+
/// Set the \a Trace object containing processor trace information of this
/// target.
///
@@ -1554,6 +1558,7 @@ class Target : public std::enable_shared_from_this<Target>,
std::string m_label;
ModuleList m_images; ///< The list of images for this process (shared
/// libraries and anything dynamically loaded).
+ SummaryStatisticsCache m_summary_statistics_cache;
SectionLoadHistory m_section_load_history;
BreakpointList m_breakpoint_list;
BreakpointList m_internal_breakpoint_list;
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index 8f72efc2299b4f..8e6ff41c08539f 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -615,7 +615,16 @@ bool ValueObject::GetSummaryAsCString(TypeSummaryImpl *summary_ptr,
m_synthetic_value->UpdateValueIfNeeded(); // the summary might depend on
// the synthetic children being
// up-to-date (e.g. ${svar%#})
- summary_ptr->FormatObject(this, destination, actual_options);
+ SummaryStatistics &summary_stats = GetExecutionContextRef()
+ .GetProcessSP()
+ ->GetTarget()
+ .GetSummaryStatisticsForProvider(GetTypeName());
+ /// Construct RAII types to time and collect data on summary creation.
+ SummaryStatistics::SummaryInvocation summary_inv(summary_stats);
+ {
+ ElapsedTime elapsed(summary_stats.GetDurationReference());
+ summary_ptr->FormatObject(this, destination, actual_options);
+ }
}
m_flags.m_is_getting_summary = false;
return !destination.empty();
diff --git a/lldb/source/Target/Statistics.cpp b/lldb/source/Target/Statistics.cpp
index 583d1524881fc3..bcb8fdbca42a3c 100644
--- a/lldb/source/Target/Statistics.cpp
+++ b/lldb/source/Target/Statistics.cpp
@@ -192,6 +192,8 @@ TargetStats::ToJSON(Target &target,
}
target_metrics_json.try_emplace("sourceMapDeduceCount",
m_source_map_deduce_count);
+ target_metrics_json.try_emplace("summaryProviderStatistics",
+ target.GetSummaryStatisticsCache().ToJSON());
return target_metrics_json;
}
@@ -408,3 +410,23 @@ llvm::json::Value DebuggerStats::ReportStatistics(
return std::move(global_stats);
}
+
+llvm::json::Value SummaryStatistics::ToJSON() const {
+ json::Object body {{
+ {"invocationCount", GetSummaryCount()},
+ {"totalTime", GetTotalTime()},
+ {"averageTime", GetAverageTime()}
+ }};
+ return json::Object{{GetName().AsCString(), std::move(body)}};
+}
+
+
+json::Value SummaryStatisticsCache::ToJSON() {
+ m_map_mutex.lock();
+ json::Array json_summary_stats;
+ for (const auto &summary_stat : m_summary_stats_map)
+ json_summary_stats.emplace_back(summary_stat.second.ToJSON());
+
+ m_map_mutex.unlock();
+ return json_summary_stats;
+}
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 129683c43f0c1a..834d48763bdff8 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -3193,6 +3193,14 @@ bool Target::SetSectionUnloaded(const lldb::SectionSP §ion_sp,
void Target::ClearAllLoadedSections() { m_section_load_history.Clear(); }
+lldb_private::SummaryStatistics& Target::GetSummaryStatisticsForProvider(lldb_private::ConstString summary_provider_name) {
+ return m_summary_statistics_cache.GetSummaryStatisticsForProviderName(summary_provider_name);
+}
+
+lldb_private::SummaryStatisticsCache& Target::GetSummaryStatisticsCache() {
+ return m_summary_statistics_cache;
+}
+
void Target::SaveScriptedLaunchInfo(lldb_private::ProcessInfo &process_info) {
if (process_info.IsScriptedProcess()) {
// Only copy scripted process launch options.
diff --git a/lldb/test/API/commands/statistics/basic/Makefile b/lldb/test/API/commands/statistics/basic/Makefile
index c9319d6e6888a4..3d0b98f13f3d7b 100644
--- a/lldb/test/API/commands/statistics/basic/Makefile
+++ b/lldb/test/API/commands/statistics/basic/Makefile
@@ -1,2 +1,2 @@
-C_SOURCES := main.c
+CXX_SOURCES := main.cpp
include Makefile.rules
diff --git a/lldb/test/API/commands/statistics/basic/TestStats.py b/lldb/test/API/commands/statistics/basic/TestStats.py
index a8ac60090a760d..85e76e526849ab 100644
--- a/lldb/test/API/commands/statistics/basic/TestStats.py
+++ b/lldb/test/API/commands/statistics/basic/TestStats.py
@@ -81,7 +81,7 @@ def get_command_stats(self, debug_stats):
def test_expressions_frame_var_counts(self):
self.build()
lldbutil.run_to_source_breakpoint(
- self, "// break here", lldb.SBFileSpec("main.c")
+ self, "// break here", lldb.SBFileSpec("main.cpp")
)
self.expect("expr patatino", substrs=["27"])
@@ -224,7 +224,7 @@ def test_default_with_run(self):
self.build()
target = self.createTestTarget()
lldbutil.run_to_source_breakpoint(
- self, "// break here", lldb.SBFileSpec("main.c")
+ self, "// break here", lldb.SBFileSpec("main.cpp")
)
debug_stats = self.get_stats()
debug_stat_keys = [
@@ -250,6 +250,7 @@ def test_default_with_run(self):
"launchOrAttachTime",
"moduleIdentifiers",
"targetCreateTime",
+ "summaryProviderStatistics"
]
self.verify_keys(stats, '"stats"', keys_exist, None)
self.assertGreater(stats["firstStopTime"], 0.0)
@@ -447,6 +448,7 @@ def test_breakpoints(self):
"targetCreateTime",
"moduleIdentifiers",
"totalBreakpointResolveTime",
+ "summaryProviderStatistics"
]
self.verify_keys(target_stats, '"stats"', keys_exist, None)
self.assertGreater(target_stats["totalBreakpointResolveTime"], 0.0)
@@ -918,3 +920,24 @@ def test_order_of_options_do_not_matter(self):
debug_stats_1,
f"The order of options '{options[0]}' and '{options[1]}' should not matter",
)
+
+ def test_summary_statistics_providers(self):
+ """
+ Test summary timing statistics is included in statistics dump when
+ a type with a summary provider exists, and is evaluated.
+ """
+
+ self.build()
+ target = self.createTestTarget()
+ lldbutil.run_to_source_breakpoint(
+ self, "// stop here", lldb.SBFileSpec("main.cpp")
+ )
+ self.expect("frame var", substrs=["hello world"])
+ stats = self.get_target_stats(self.get_stats())
+ self.assertIn("summaryProviderStatistics", stats)
+ summary_providers = stats["summaryProviderStatistics"]
+ # We don't want to take a dependency on the type name, so we just look
+ # for string and that it was called once.
+ summary_provider_str = str(summary_providers)
+ self.assertIn("string", summary_provider_str)
+ self.assertIn("'invocationCount': 1", summary_provider_str)
diff --git a/lldb/test/API/commands/statistics/basic/main.c b/lldb/test/API/commands/statistics/basic/main.cpp
similarity index 52%
rename from lldb/test/API/commands/statistics/basic/main.c
rename to lldb/test/API/commands/statistics/basic/main.cpp
index 2c5b4f5f098ee4..3e3f34421eca30 100644
--- a/lldb/test/API/commands/statistics/basic/main.c
+++ b/lldb/test/API/commands/statistics/basic/main.cpp
@@ -1,7 +1,13 @@
// Test that the lldb command `statistics` works.
+#include <string>
+
+void foo() {
+ std::string str = "hello world";
+ str += "\n"; // stop here
+}
int main(void) {
int patatino = 27;
-
+ foo();
return 0; // break here
}
|
✅ With the latest revision this PR passed the Python code formatter. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
Stats around this area could definitely be interesting to explore, thanks for looking into this! |
#include <string> | ||
|
||
void foo() { | ||
std::string str = "hello world"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also test a type that uses a template as some summary providers match based on a regex like:
std::vector<int> ints = {1, 2, 3};
std::vector<float> floats = {1.0, 2.0, 3.0};
From debugging we can see:
(lldb) type summary info ints
summary applied to (std::vector<int>) ints is: (show children) (hide value) libc++ std::vector summary provider
And the summary definition looks like:
(lldb) type summary list
...
^std::vector<.+>(( )?&)?$: `size=${svar%#}` (show children) (hide value)
...
So the summary name we want to see in our resulting map is probably "^std::vector<.+>(( )?&)?$"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a basic template test, and made sure the formatter was invoked, and I added a different test for the two vector cases.
I did not test nested summaries, and will add it if desired.
lldb/source/Core/ValueObject.cpp
Outdated
@@ -602,7 +602,7 @@ bool ValueObject::GetSummaryAsCString(TypeSummaryImpl *summary_ptr, | |||
actual_options.SetLanguage(GetPreferredDisplayLanguage()); | |||
|
|||
// this is a hot path in code and we prefer to avoid setting this string all | |||
// too often also clearing out other information that we might care to see in | |||
// too often also clearing out other information that we might care` to see in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this extra character that was added
5993716
to
7b4b7fc
Compare
// Basic RAII class to increment the summary count when the call is complete. | ||
// In the future this can be extended to collect information about the | ||
// elapsed time for a single request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Basic RAII class to increment the summary count when the call is complete. | |
// In the future this can be extended to collect information about the | |
// elapsed time for a single request. | |
/// Basic RAII class to increment the summary count when the call is complete. | |
/// In the future this can be extended to collect information about the | |
/// elapsed time for a single request. |
2a9443c
to
4a2f234
Compare
lldb/source/Core/ValueObject.cpp
Outdated
|
||
TargetSP target_sp = GetExecutionContextRef().GetTargetSP(); | ||
if (target_sp) { | ||
// Get Shared pointer to the summary statistics container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Get Shared pointer to the summary statistics container |
Nit: don't need the comment here
/// Get the SummaryStatistics object for a given provider name, or insert | ||
/// if statistics for that provider is not in the map. | ||
SummaryStatisticsSP | ||
GetSummaryStatisticsForProviderName(lldb_private::TypeSummaryImpl &provider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it's mostly personal preference, thanks for addressing all the comments so far. This approach seems fine to me too (though I've had enough time to let it sink in, not sure it'd be straightforward to reason about for someone new to this).
std::atomic<uint64_t> m_count; | ||
}; | ||
|
||
typedef std::shared_ptr<SummaryStatistics> SummaryStatisticsSP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the exact policy is but i think we define all of these in lldb-forward.h
(given you expose it in the header here anyway, probably best to move it there, but will defer to @clayborg )
@@ -0,0 +1,35 @@ | |||
// Test that the lldb command `statistics` works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not aware of anything like this off-the-top. We'd want a test that access the same Target
from multiple threads and formats types. Maybe having some callback tied to several threads within the target which trigger a ValueObject::GetSummaryAsCString
call. @clayborg @jimingham might have some more concrete ideas on how to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (left some minor comments)
@Michael137 implemented your feedback, and I got gcf to work. I'm going to let CI run and then merge. Thanks for your patience! |
…mes they're summary strings.
/// Get the SummaryStatistics object for a given provider name, or insert | ||
/// if statistics for that provider is not in the map. | ||
SummaryStatisticsSP | ||
GetSummaryStatisticsForProviderName(lldb_private::TypeSummaryImpl &provider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with your suggestion the RAII object would need to copy the string so that it could call Update
at the and to find the stats and then update the time. This also means the RAII object needs to hold a reference to the SummaryStatisticsCache
global object, which could get destructed as nothing is keeping it alive. So we need to have the RAII object copy the string and get a weak pointer to the SummaryStatisticsCache so it can safely update the results in the end.
This current solution hands out a SummaryStatisticsSP
to the RAII object which will keep it alive so it can get updated, and if the SummaryStatisticsCache
goes away, it is ok because we have a strong reference. Both solutions only do 1 lookup. std::shared_ptr<T>
objects are thread safe.
The StringMap requires a value that can be copied when the map grows and re-allocation happen, so if we don't use a shared pointer, then we need SummaryStatistics
to have a move operator.
So I don't mind this solution, but would like to hear other opinions
|
||
/// Get the name of the kind of Summary Provider, either c++, summary string, | ||
/// script or python. | ||
virtual std::string GetSummaryKindName() = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a string or an enum? I would vote for an lldb_internal
enum. That way plug-ins know what to choose form and don't make up random stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to the enum, but I think if we're using an enum we should leverage 'TypeSummaryKind`, the existing kind enum and we should to string that.
My question is how we handle script, currently I check if it's a .py
and if it is we call it Python. Do we want to default to Python for all scripts?
@Michael137 I talked to Greg offline about this, but do we support any SWIG interfaced languages other than Python? I believe I've seen Lua in the wild. If that's the case I'd like to keep the differentiation, but for this first patch we could get away with just Python
3796858
to
76a3103
Compare
76a3103
to
f63080b
Compare
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/2290 Here is the relevant piece of the build log for the reference
|
Investigating the windows issue |
Follow up to #102708, the tests are failing for windows. There is a large variance in these tests between summary strings and built in types. I'm disabling these test for windows, and will add windows specific tests as a follow up to this.
…providers within lldb (llvm#102708) This PR adds a statistics provider cache, which allows an individual target to keep a rolling tally of it's total time and number of invocations for a given summary provider. This information is then available in statistics dump to help slow summary providers, and gleam more into insight into LLDB's time use. (cherry picked from commit 22144e2)
This PR adds a statistics provider cache, which allows an individual target to keep a rolling tally of it's total time and number of invocations for a given summary provider. This information is then available in statistics dump to help slow summary providers, and gleam more into insight into LLDB's time use.