Merge "tp: fix a bunch of bugs in ProtoLogParser" into main
diff --git a/src/trace_processor/importers/proto/winscope/BUILD.gn b/src/trace_processor/importers/proto/winscope/BUILD.gn
index 47e5cb4..93f9d1b 100644
--- a/src/trace_processor/importers/proto/winscope/BUILD.gn
+++ b/src/trace_processor/importers/proto/winscope/BUILD.gn
@@ -41,6 +41,8 @@
"../../../../../protos/perfetto/trace/android:zero",
"../../../../../protos/perfetto/trace/interned_data:zero",
"../../../../../protos/perfetto/trace/profiling:zero",
+ "../../../../protozero",
+ "../../../containers",
"../../../storage",
"../../../tables",
"../../../types",
diff --git a/src/trace_processor/importers/proto/winscope/protolog_messages_tracker.cc b/src/trace_processor/importers/proto/winscope/protolog_messages_tracker.cc
index caf481d..1cfd530 100644
--- a/src/trace_processor/importers/proto/winscope/protolog_messages_tracker.cc
+++ b/src/trace_processor/importers/proto/winscope/protolog_messages_tracker.cc
@@ -14,16 +14,15 @@
* limitations under the License.
*/
-#include "protolog_messages_tracker.h"
-#include "perfetto/ext/base/crash_keys.h"
-#include "src/trace_processor/importers/common/process_tracker.h"
-#include "src/trace_processor/storage/metadata.h"
-#include "src/trace_processor/types/trace_processor_context.h"
+#include "src/trace_processor/importers/proto/winscope/protolog_messages_tracker.h"
-namespace perfetto {
-namespace trace_processor {
-ProtoLogMessagesTracker::ProtoLogMessagesTracker() {}
+#include <cstdint>
+#include <optional>
+#include <vector>
+namespace perfetto::trace_processor {
+
+ProtoLogMessagesTracker::ProtoLogMessagesTracker() = default;
ProtoLogMessagesTracker::~ProtoLogMessagesTracker() = default;
void ProtoLogMessagesTracker::TrackMessage(
@@ -34,18 +33,14 @@
.first->emplace_back(tracked_protolog_message);
}
-const std::optional<
- std::vector<ProtoLogMessagesTracker::TrackedProtoLogMessage>*>
+std::optional<std::vector<ProtoLogMessagesTracker::TrackedProtoLogMessage>*>
ProtoLogMessagesTracker::GetTrackedMessagesByMessageId(uint64_t message_id) {
- auto tracked_messages = tracked_protolog_messages.Find(message_id);
-
+ auto* tracked_messages = tracked_protolog_messages.Find(message_id);
if (tracked_messages == nullptr) {
// No tracked messages found for this id
return std::nullopt;
}
-
return tracked_messages;
}
-} // namespace trace_processor
-} // namespace perfetto
+} // namespace perfetto::trace_processor
diff --git a/src/trace_processor/importers/proto/winscope/protolog_messages_tracker.h b/src/trace_processor/importers/proto/winscope/protolog_messages_tracker.h
index 7b1eb9b..de0285f 100644
--- a/src/trace_processor/importers/proto/winscope/protolog_messages_tracker.h
+++ b/src/trace_processor/importers/proto/winscope/protolog_messages_tracker.h
@@ -17,12 +17,18 @@
#ifndef SRC_TRACE_PROCESSOR_IMPORTERS_PROTO_WINSCOPE_PROTOLOG_MESSAGES_TRACKER_H_
#define SRC_TRACE_PROCESSOR_IMPORTERS_PROTO_WINSCOPE_PROTOLOG_MESSAGES_TRACKER_H_
-#include "perfetto/trace_processor/basic_types.h"
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <vector>
+
+#include "perfetto/ext/base/flat_hash_map.h"
#include "src/trace_processor/storage/trace_storage.h"
+#include "src/trace_processor/tables/winscope_tables_py.h"
+#include "src/trace_processor/types/destructible.h"
#include "src/trace_processor/types/trace_processor_context.h"
-namespace perfetto {
-namespace trace_processor {
+namespace perfetto::trace_processor {
class ProtoLogMessagesTracker : public Destructible {
public:
@@ -49,8 +55,7 @@
}
void TrackMessage(TrackedProtoLogMessage tracked_protolog_message);
- const std::optional<
- std::vector<ProtoLogMessagesTracker::TrackedProtoLogMessage>*>
+ std::optional<std::vector<ProtoLogMessagesTracker::TrackedProtoLogMessage>*>
GetTrackedMessagesByMessageId(uint64_t message_id);
private:
@@ -58,7 +63,6 @@
tracked_protolog_messages;
};
-} // namespace trace_processor
-} // namespace perfetto
+} // namespace perfetto::trace_processor
#endif // SRC_TRACE_PROCESSOR_IMPORTERS_PROTO_WINSCOPE_PROTOLOG_MESSAGES_TRACKER_H_
diff --git a/src/trace_processor/importers/proto/winscope/protolog_parser.cc b/src/trace_processor/importers/proto/winscope/protolog_parser.cc
index 304a829..a95777d 100644
--- a/src/trace_processor/importers/proto/winscope/protolog_parser.cc
+++ b/src/trace_processor/importers/proto/winscope/protolog_parser.cc
@@ -15,23 +15,33 @@
*/
#include "src/trace_processor/importers/proto/winscope/protolog_parser.h"
-#include "src/trace_processor/importers/proto/winscope/protolog_messages_tracker.h"
+#include <cinttypes>
+#include <cstddef>
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "perfetto/ext/base/flat_hash_map.h"
#include "perfetto/ext/base/string_utils.h"
+#include "perfetto/ext/base/string_view.h"
+#include "perfetto/protozero/field.h"
#include "protos/perfetto/trace/android/protolog.pbzero.h"
#include "protos/perfetto/trace/interned_data/interned_data.pbzero.h"
#include "protos/perfetto/trace/profiling/profile_common.pbzero.h"
#include "protos/perfetto/trace/profiling/profile_packet.pbzero.h"
-#include "src/trace_processor/importers/common/args_tracker.h"
+#include "src/trace_processor/containers/string_pool.h"
#include "src/trace_processor/importers/proto/packet_sequence_state_generation.h"
+#include "src/trace_processor/importers/proto/winscope/protolog_messages_tracker.h"
#include "src/trace_processor/importers/proto/winscope/winscope.descriptor.h"
-#include "src/trace_processor/importers/proto/winscope/winscope_args_parser.h"
+#include "src/trace_processor/storage/stats.h"
#include "src/trace_processor/storage/trace_storage.h"
+#include "src/trace_processor/tables/winscope_tables_py.h"
#include "src/trace_processor/types/trace_processor_context.h"
-#include <sstream>
-namespace perfetto {
-namespace trace_processor {
+namespace perfetto::trace_processor {
enum ProtoLogLevel : int32_t {
DEBUG = 1,
@@ -79,44 +89,27 @@
std::vector<std::string> string_params;
if (protolog_message.has_str_param_iids()) {
- if (sequence_state->state()->IsIncrementalStateValid()) {
- for (auto it = protolog_message.str_param_iids(); it; ++it) {
- auto decoder =
- sequence_state->state()
- ->current_generation()
- ->LookupInternedMessage<protos::pbzero::InternedData::
- kProtologStringArgsFieldNumber,
- protos::pbzero::InternedString>(
- it.field().as_uint32());
-
- if (!decoder) {
- // This shouldn't happen since we already checked the incremental
- // state is valid.
- string_params.emplace_back("<ERROR>");
- context_->storage->IncrementStats(
- stats::winscope_protolog_missing_interned_arg_parse_errors);
- continue;
- }
-
- string_params.emplace_back(decoder->str().ToStdString());
+ for (auto it = protolog_message.str_param_iids(); it; ++it) {
+ auto* decoder = sequence_state->LookupInternedMessage<
+ protos::pbzero::InternedData::kProtologStringArgsFieldNumber,
+ protos::pbzero::InternedString>(it.field().as_uint32());
+ if (!decoder) {
+ // This shouldn't happen since we already checked the incremental
+ // state is valid.
+ string_params.emplace_back("<ERROR>");
+ context_->storage->IncrementStats(
+ stats::winscope_protolog_missing_interned_arg_parse_errors);
+ continue;
}
- } else {
- // If the incremental state is not valid we will not be able to decode
- // the interned strings correctly with 100% certainty so we will provide
- // string parameters that are not decoded.
- string_params.emplace_back("<MISSING_STR_ARG>");
+ string_params.emplace_back(decoder->str().ToStdString());
}
}
std::optional<StringId> stacktrace = std::nullopt;
if (protolog_message.has_stacktrace_iid()) {
- auto stacktrace_decoder =
- sequence_state->state()
- ->current_generation()
- ->LookupInternedMessage<
- protos::pbzero::InternedData::kProtologStacktraceFieldNumber,
- protos::pbzero::InternedString>(
- protolog_message.stacktrace_iid());
+ auto* stacktrace_decoder = sequence_state->LookupInternedMessage<
+ protos::pbzero::InternedData::kProtologStacktraceFieldNumber,
+ protos::pbzero::InternedString>(protolog_message.stacktrace_iid());
if (!stacktrace_decoder) {
// This shouldn't happen since we already checked the incremental
@@ -135,7 +128,7 @@
tables::ProtoLogTable::Row row;
auto row_id = protolog_table->Insert(row).id;
- auto protolog_message_tracker =
+ auto* protolog_message_tracker =
ProtoLogMessagesTracker::GetOrCreate(context_);
struct ProtoLogMessagesTracker::TrackedProtoLogMessage tracked_message = {
protolog_message.message_id(),
@@ -154,13 +147,13 @@
protos::pbzero::ProtoLogViewerConfig::Decoder protolog_viewer_config(blob);
- std::unordered_map<uint32_t, std::string> group_tags;
+ base::FlatHashMap<uint32_t, std::string> group_tags;
for (auto it = protolog_viewer_config.groups(); it; ++it) {
protos::pbzero::ProtoLogViewerConfig::Group::Decoder group(*it);
- group_tags.insert({group.id(), group.tag().ToStdString()});
+ group_tags.Insert(group.id(), group.tag().ToStdString());
}
- auto protolog_message_tracker =
+ auto* protolog_message_tracker =
ProtoLogMessagesTracker::GetOrCreate(context_);
for (auto it = protolog_viewer_config.messages(); it; ++it) {
@@ -172,7 +165,7 @@
message_data.message_id());
if (tracked_messages_opt.has_value()) {
- auto group_tag = group_tags.find(message_data.group_id())->second;
+ auto* group_tag = group_tags.Find(message_data.group_id());
for (const auto& tracked_message : *tracked_messages_opt.value()) {
auto formatted_message = FormatMessage(
@@ -211,7 +204,8 @@
}
row.set_level(level);
- auto tag = context_->storage->InternString(base::StringView(group_tag));
+ auto tag =
+ context_->storage->InternString(base::StringView(*group_tag));
row.set_tag(tag);
auto message = context_->storage->InternString(
@@ -227,7 +221,7 @@
}
std::string ProtoLogParser::FormatMessage(
- const std::string message,
+ const std::string& message,
const std::vector<int64_t>& sint64_params,
const std::vector<double>& double_params,
const std::vector<bool>& boolean_params,
@@ -282,7 +276,7 @@
break;
}
case 's': {
- formatted_message.append(str_params_itr->c_str());
+ formatted_message.append(*str_params_itr);
++str_params_itr;
break;
}
@@ -307,5 +301,4 @@
return formatted_message;
}
-} // namespace trace_processor
-} // namespace perfetto
+} // namespace perfetto::trace_processor
diff --git a/src/trace_processor/importers/proto/winscope/protolog_parser.h b/src/trace_processor/importers/proto/winscope/protolog_parser.h
index 97d9183..4c0db6f 100644
--- a/src/trace_processor/importers/proto/winscope/protolog_parser.h
+++ b/src/trace_processor/importers/proto/winscope/protolog_parser.h
@@ -17,15 +17,15 @@
#ifndef SRC_TRACE_PROCESSOR_IMPORTERS_PROTO_WINSCOPE_PROTOLOG_PARSER_H_
#define SRC_TRACE_PROCESSOR_IMPORTERS_PROTO_WINSCOPE_PROTOLOG_PARSER_H_
-#include "protos/perfetto/trace/android/protolog.pbzero.h"
-#include "src/trace_processor/importers/proto/packet_sequence_state.h"
+#include <cstdint>
+#include <string>
+#include <vector>
+
#include "src/trace_processor/storage/trace_storage.h"
#include "src/trace_processor/util/descriptors.h"
#include "src/trace_processor/util/proto_to_args_parser.h"
-namespace perfetto {
-
-namespace trace_processor {
+namespace perfetto::trace_processor {
class TraceProcessorContext;
@@ -38,15 +38,12 @@
void ParseProtoLogViewerConfig(protozero::ConstBytes);
private:
- std::string FormatMessage(const std::string message,
+ std::string FormatMessage(const std::string& message,
const std::vector<int64_t>& sint64_params,
const std::vector<double>& double_params,
const std::vector<bool>& boolean_params,
const std::vector<std::string>& string_params);
- static constexpr auto* kProtoLogMessageProtoName =
- "perfetto.protos.ProtoLogMessage";
-
TraceProcessorContext* const context_;
DescriptorPool pool_;
util::ProtoToArgsParser args_parser_;
@@ -59,7 +56,6 @@
const StringId log_level_wtf_string_id_;
const StringId log_level_unknown_string_id_;
};
-} // namespace trace_processor
-} // namespace perfetto
+} // namespace perfetto::trace_processor
#endif // SRC_TRACE_PROCESSOR_IMPORTERS_PROTO_WINSCOPE_PROTOLOG_PARSER_H_
