Merge "Add interning support to ProtoToArgsParser."
diff --git a/src/trace_processor/importers/proto/packet_sequence_state.cc b/src/trace_processor/importers/proto/packet_sequence_state.cc
index c63c2d7..2eb2350 100644
--- a/src/trace_processor/importers/proto/packet_sequence_state.cc
+++ b/src/trace_processor/importers/proto/packet_sequence_state.cc
@@ -51,5 +51,21 @@
message_size) == 0));
}
+InternedMessageView* PacketSequenceStateGeneration::GetInternedMessageView(
+ uint32_t field_id,
+ uint64_t iid) {
+ auto field_it = interned_data_.find(field_id);
+ if (field_it != interned_data_.end()) {
+ auto* message_map = &field_it->second;
+ auto it = message_map->find(iid);
+ if (it != message_map->end()) {
+ return &it->second;
+ }
+ }
+ state_->context()->storage->IncrementStats(
+ stats::interned_data_tokenizer_errors);
+ return nullptr;
+}
+
} // namespace trace_processor
} // namespace perfetto
diff --git a/src/trace_processor/importers/proto/packet_sequence_state.h b/src/trace_processor/importers/proto/packet_sequence_state.h
index 8ceb4e5..fc5288c 100644
--- a/src/trace_processor/importers/proto/packet_sequence_state.h
+++ b/src/trace_processor/importers/proto/packet_sequence_state.h
@@ -157,8 +157,7 @@
template <uint32_t FieldId, typename MessageType>
typename MessageType::Decoder* LookupInternedMessage(uint64_t iid);
- template <uint32_t FieldId>
- InternedMessageView* GetInternedMessageView(uint64_t iid);
+ InternedMessageView* GetInternedMessageView(uint32_t field_id, uint64_t iid);
// Returns |nullptr| if no defaults were set.
InternedMessageView* GetTracePacketDefaultsView() {
if (!trace_packet_defaults_)
@@ -354,26 +353,10 @@
SequenceStackProfileTracker sequence_stack_profile_tracker_;
};
-template <uint32_t FieldId>
-InternedMessageView* PacketSequenceStateGeneration::GetInternedMessageView(
- uint64_t iid) {
- auto field_it = interned_data_.find(FieldId);
- if (field_it != interned_data_.end()) {
- auto* message_map = &field_it->second;
- auto it = message_map->find(iid);
- if (it != message_map->end()) {
- return &it->second;
- }
- }
- state_->context()->storage->IncrementStats(
- stats::interned_data_tokenizer_errors);
- return nullptr;
-}
-
template <uint32_t FieldId, typename MessageType>
typename MessageType::Decoder*
PacketSequenceStateGeneration::LookupInternedMessage(uint64_t iid) {
- auto* interned_message_view = GetInternedMessageView<FieldId>(iid);
+ auto* interned_message_view = GetInternedMessageView(FieldId, iid);
if (!interned_message_view)
return nullptr;
diff --git a/src/trace_processor/importers/proto/profile_packet_utils.h b/src/trace_processor/importers/proto/profile_packet_utils.h
index eaa561e..2dde388 100644
--- a/src/trace_processor/importers/proto/profile_packet_utils.h
+++ b/src/trace_processor/importers/proto/profile_packet_utils.h
@@ -174,8 +174,8 @@
base::Optional<SequenceStackProfileTracker::SourceCallstack> GetCallstack(
SequenceStackProfileTracker::SourceCallstackId iid) const override {
- auto* interned_message_view = seq_state_->GetInternedMessageView<
- protos::pbzero::InternedData::kCallstacksFieldNumber>(iid);
+ auto* interned_message_view = seq_state_->GetInternedMessageView(
+ protos::pbzero::InternedData::kCallstacksFieldNumber, iid);
if (!interned_message_view)
return base::nullopt;
protos::pbzero::Callstack::Decoder decoder(
diff --git a/src/trace_processor/importers/proto/track_event_parser.cc b/src/trace_processor/importers/proto/track_event_parser.cc
index 5ab28b2..8ad8177 100644
--- a/src/trace_processor/importers/proto/track_event_parser.cc
+++ b/src/trace_processor/importers/proto/track_event_parser.cc
@@ -113,14 +113,11 @@
Variadic::Json(storage_.InternString(value)));
}
- PacketSequenceStateGeneration& sequence_state() const {
- return sequence_state_;
+ InternedMessageView* GetInternedMessageView(uint32_t field_id,
+ uint64_t iid) final {
+ return sequence_state_.GetInternedMessageView(field_id, iid);
}
- TraceStorage& storage() const { return storage_; }
-
- BoundInserter& inserter() const { return inserter_; }
-
private:
BoundInserter& inserter_;
TraceStorage& storage_;
@@ -133,31 +130,20 @@
std::string prefix,
const protozero::Field& field,
util::ProtoToArgsParser::Delegate& delegate) {
- TrackEventArgsParser& parser = static_cast<TrackEventArgsParser&>(delegate);
- auto* decoder =
- parser.sequence_state()
- .LookupInternedMessage<
- protos::pbzero::InternedData::kSourceLocationsFieldNumber,
- protos::pbzero::SourceLocation>(field.as_uint64());
+ auto* decoder = delegate.GetInternedMessage(
+ protos::pbzero::InternedData::kSourceLocations, field.as_uint64());
if (!decoder) {
// Lookup failed fall back on default behaviour which will just put
// the source_location_iid into the args table.
return base::nullopt;
}
- TraceStorage& storage = parser.storage();
- BoundInserter& inserter = parser.inserter();
-
- inserter.AddArg(storage.InternString(base::StringView(prefix + ".file_name")),
- Variadic::String(storage.InternString(
- base::StringView(decoder->file_name()))));
- inserter.AddArg(
- storage.InternString(base::StringView(prefix + ".function_name")),
- Variadic::String(
- storage.InternString(base::StringView(decoder->function_name()))));
- inserter.AddArg(
- storage.InternString(base::StringView(prefix + ".line_number")),
- Variadic::Integer(decoder->line_number()));
+ delegate.AddString(util::ProtoToArgsParser::Key(prefix + ".file_name"),
+ decoder->file_name());
+ delegate.AddString(util::ProtoToArgsParser::Key(prefix + ".function_name"),
+ decoder->function_name());
+ delegate.AddInteger(util::ProtoToArgsParser::Key(prefix + ".line_number"),
+ decoder->line_number());
return base::OkStatus();
}
diff --git a/src/trace_processor/util/BUILD.gn b/src/trace_processor/util/BUILD.gn
index ed1a8dd..02887ac 100644
--- a/src/trace_processor/util/BUILD.gn
+++ b/src/trace_processor/util/BUILD.gn
@@ -67,15 +67,23 @@
"proto_to_args_parser.h",
]
deps = [
- ":descriptors",
- ":util",
"../../../gn:default_deps",
"../../../protos/perfetto/common:zero",
+ "../../../protos/perfetto/trace/interned_data:zero",
"../../../protos/perfetto/trace_processor:zero",
- "../../base",
"../../protozero",
"../importers:gen_cc_track_event_descriptor",
]
+
+ public_deps = [
+ ":descriptors",
+ ":util",
+ "../../base",
+
+ # TODO(altimin): Move InternedMessageView and TraceBlobView here and remove
+ # this dependency.
+ "../importers/common",
+ ]
}
source_set("unittests") {
@@ -96,5 +104,9 @@
"../../protozero",
"../../protozero:testing_messages_zero",
"../importers:gen_cc_track_event_descriptor",
+
+ # TODO(altimin): Move InternedMessageView and TraceBlobView here and remove
+ # this dependency.
+ "..:storage_minimal",
]
}
diff --git a/src/trace_processor/util/proto_to_args_parser.cc b/src/trace_processor/util/proto_to_args_parser.cc
index b578ea6..6fec35e 100644
--- a/src/trace_processor/util/proto_to_args_parser.cc
+++ b/src/trace_processor/util/proto_to_args_parser.cc
@@ -57,6 +57,12 @@
} // namespace
+ProtoToArgsParser::Key::Key() = default;
+ProtoToArgsParser::Key::Key(const std::string& k) : flat_key(k), key(k) {}
+ProtoToArgsParser::Key::Key(const std::string& fk, const std::string& k)
+ : flat_key(fk), key(k) {}
+ProtoToArgsParser::Key::~Key() = default;
+
ProtoToArgsParser::Delegate::~Delegate() = default;
ProtoToArgsParser::ProtoToArgsParser(const DescriptorPool& pool) : pool_(pool) {
diff --git a/src/trace_processor/util/proto_to_args_parser.h b/src/trace_processor/util/proto_to_args_parser.h
index 9c1dff6..f99179c 100644
--- a/src/trace_processor/util/proto_to_args_parser.h
+++ b/src/trace_processor/util/proto_to_args_parser.h
@@ -19,10 +19,15 @@
#include "perfetto/base/status.h"
#include "perfetto/protozero/field.h"
+#include "protos/perfetto/trace/interned_data/interned_data.pbzero.h"
#include "src/trace_processor/util/descriptors.h"
namespace perfetto {
namespace trace_processor {
+
+// TODO(altimin): Move InternedMessageView into trace_processor/util.
+class InternedMessageView;
+
namespace util {
// ProtoToArgsParser encapsulates the process of taking an arbitrary proto and
@@ -58,8 +63,13 @@
explicit ProtoToArgsParser(const DescriptorPool& descriptor_pool);
struct Key {
- std::string key;
+ Key(const std::string& flat_key, const std::string& key);
+ Key(const std::string& key);
+ Key();
+ ~Key();
+
std::string flat_key;
+ std::string key;
};
class Delegate {
@@ -75,6 +85,25 @@
virtual void AddBoolean(const Key& key, bool value) = 0;
virtual void AddJson(const Key& key,
const protozero::ConstChars& value) = 0;
+
+ template <typename FieldMetadata>
+ typename FieldMetadata::cpp_field_type::Decoder* GetInternedMessage(
+ protozero::proto_utils::internal::FieldMetadataHelper<FieldMetadata>,
+ uint64_t iid) {
+ static_assert(std::is_base_of<protozero::proto_utils::FieldMetadataBase,
+ FieldMetadata>::value,
+ "Field metadata should be a subclass of FieldMetadataBase");
+ static_assert(std::is_same<typename FieldMetadata::message_type,
+ protos::pbzero::InternedData>::value,
+ "Field should belong to InternedData proto");
+ return GetInternedMessageView(FieldMetadata::kFieldId, iid)
+ ->template GetOrCreateDecoder<
+ typename FieldMetadata::cpp_field_type>();
+ }
+
+ protected:
+ virtual InternedMessageView* GetInternedMessageView(uint32_t field_id,
+ uint64_t iid) = 0;
};
using ParsingOverride =
diff --git a/src/trace_processor/util/proto_to_args_parser_unittest.cc b/src/trace_processor/util/proto_to_args_parser_unittest.cc
index 59aedc4..89ee945 100644
--- a/src/trace_processor/util/proto_to_args_parser_unittest.cc
+++ b/src/trace_processor/util/proto_to_args_parser_unittest.cc
@@ -21,6 +21,8 @@
#include "protos/perfetto/common/descriptor.pbzero.h"
#include "protos/perfetto/trace/track_event/source_location.pbzero.h"
#include "src/protozero/test/example_proto/test_messages.pbzero.h"
+#include "src/trace_processor/importers/common/trace_blob_view.h"
+#include "src/trace_processor/importers/proto/packet_sequence_state.h"
#include "src/trace_processor/test_messages.descriptor.h"
#include "test/gtest_and_gmock.h"
@@ -45,6 +47,11 @@
const std::vector<std::string>& args() const { return args_; }
+ void AddInternedSourceLocation(uint64_t iid, TraceBlobView data) {
+ interned_source_locations_[iid] = std::unique_ptr<InternedMessageView>(
+ new InternedMessageView(std::move(data)));
+ }
+
private:
using Key = ProtoToArgsParser::Key;
@@ -92,7 +99,16 @@
args_.push_back(ss.str());
}
+ InternedMessageView* GetInternedMessageView(uint32_t field_id,
+ uint64_t iid) override {
+ if (field_id != protos::pbzero::InternedData::kSourceLocationsFieldNumber)
+ return nullptr;
+ return interned_source_locations_.at(iid).get();
+ }
+
std::vector<std::string> args_;
+ std::map<uint64_t, std::unique_ptr<InternedMessageView>>
+ interned_source_locations_;
};
TEST_F(ProtoToArgsParserTest, EnsureTestMessageProtoParses) {
@@ -289,6 +305,63 @@
"super_nested.value_c super_nested.value_c 3"));
}
+TEST_F(ProtoToArgsParserTest, LookingUpInternedStateParsingOverride) {
+ using namespace protozero::test::protos::pbzero;
+ // The test proto, we will use |value_c| as the source_location iid.
+ protozero::HeapBuffered<NestedA> msg{kChunkSize, kChunkSize};
+ msg->set_super_nested()->set_value_c(3);
+ auto binary_proto = msg.SerializeAsArray();
+
+ // The interned source location.
+ protozero::HeapBuffered<protos::pbzero::SourceLocation> src_loc{kChunkSize,
+ kChunkSize};
+ const uint64_t kIid = 3;
+ src_loc->set_iid(kIid);
+ src_loc->set_file_name("test_file_name");
+ // We need to update sequence_state to point to it.
+ auto binary_data = src_loc.SerializeAsArray();
+ std::unique_ptr<uint8_t[]> buffer(new uint8_t[binary_data.size()]);
+ for (size_t i = 0; i < binary_data.size(); ++i) {
+ buffer.get()[i] = binary_data[i];
+ }
+ TraceBlobView blob(std::move(buffer), 0, binary_data.size());
+ AddInternedSourceLocation(kIid, std::move(blob));
+
+ DescriptorPool pool;
+ auto status = pool.AddFromFileDescriptorSet(kTestMessagesDescriptor.data(),
+ kTestMessagesDescriptor.size());
+ ASSERT_TRUE(status.ok()) << "Failed to parse kTestMessagesDescriptor: "
+ << status.message();
+
+ ProtoToArgsParser parser(pool);
+ // Now we override the behaviour of |value_c| so we can expand the iid into
+ // multiple args rows.
+ parser.AddParsingOverride(
+ "super_nested.value_c",
+ [](const protozero::Field& field, ProtoToArgsParser::Delegate& delegate)
+ -> base::Optional<base::Status> {
+ auto* decoder = delegate.GetInternedMessage(
+ protos::pbzero::InternedData::kSourceLocations, field.as_uint64());
+ if (!decoder) {
+ // Lookup failed fall back on default behaviour.
+ return base::nullopt;
+ }
+ delegate.AddString(ProtoToArgsParser::Key("file_name"),
+ protozero::ConstChars{"file", 4});
+ delegate.AddInteger(ProtoToArgsParser::Key("line_number"), 2);
+ return base::OkStatus();
+ });
+
+ status = parser.ParseMessage(
+ protozero::ConstBytes{binary_proto.data(), binary_proto.size()},
+ ".protozero.test.protos.NestedA", nullptr, *this);
+ EXPECT_TRUE(status.ok())
+ << "InternProtoFieldsIntoArgsTable failed with error: "
+ << status.message();
+ EXPECT_THAT(args(), testing::ElementsAre("file_name file_name file",
+ "line_number line_number 2"));
+}
+
} // namespace
} // namespace util
} // namespace trace_processor