tp: add power rail raw name to track not to counter
Previously, we were adding the power rail raw name to every counter
event which is wasteful because it is same for every item in the track.
Instead, add the raw name to the counter arg set instead.
Also while we're here, add the subsystem name as well.
Change-Id: I709f121bbcb5fd7255afa91eb445b2050468da81
Bug: 226403822
diff --git a/src/trace_processor/importers/common/track_tracker.cc b/src/trace_processor/importers/common/track_tracker.cc
index 36a282d..8a3ebaa 100644
--- a/src/trace_processor/importers/common/track_tracker.cc
+++ b/src/trace_processor/importers/common/track_tracker.cc
@@ -193,6 +193,7 @@
}
TrackId TrackTracker::InternGlobalCounterTrack(StringId name,
+ SetArgsCallback callback,
StringId unit,
StringId description) {
auto it = global_counter_tracks_by_name_.find(name);
@@ -206,6 +207,10 @@
TrackId track =
context_->storage->mutable_counter_track_table()->Insert(row).id;
global_counter_tracks_by_name_[name] = track;
+ if (callback) {
+ auto inserter = context_->args_tracker->AddArgsTo(track);
+ callback(inserter);
+ }
return track;
}
diff --git a/src/trace_processor/importers/common/track_tracker.h b/src/trace_processor/importers/common/track_tracker.h
index d51d41a..a932901 100644
--- a/src/trace_processor/importers/common/track_tracker.h
+++ b/src/trace_processor/importers/common/track_tracker.h
@@ -17,6 +17,7 @@
#ifndef SRC_TRACE_PROCESSOR_IMPORTERS_COMMON_TRACK_TRACKER_H_
#define SRC_TRACE_PROCESSOR_IMPORTERS_COMMON_TRACK_TRACKER_H_
+#include "src/trace_processor/importers/common/args_tracker.h"
#include "src/trace_processor/storage/trace_storage.h"
#include "src/trace_processor/types/trace_processor_context.h"
@@ -26,6 +27,8 @@
// Tracks and stores tracks based on track types, ids and scopes.
class TrackTracker {
public:
+ using SetArgsCallback = std::function<void(ArgsTracker::BoundInserter&)>;
+
explicit TrackTracker(TraceProcessorContext*);
// Interns a thread track into the storage.
@@ -65,6 +68,7 @@
// Interns a global counter track into the storage.
TrackId InternGlobalCounterTrack(StringId name,
+ SetArgsCallback = {},
StringId unit = kNullStringId,
StringId description = kNullStringId);
diff --git a/src/trace_processor/importers/ftrace/ftrace_parser.cc b/src/trace_processor/importers/ftrace/ftrace_parser.cc
index 3861e92..660834f 100644
--- a/src/trace_processor/importers/ftrace/ftrace_parser.cc
+++ b/src/trace_processor/importers/ftrace/ftrace_parser.cc
@@ -1645,7 +1645,7 @@
if (pid == 0) {
// Pid 0 is used to indicate the global total
track = context_->track_tracker->InternGlobalCounterTrack(
- gpu_mem_total_name_id_, gpu_mem_total_unit_id_,
+ gpu_mem_total_name_id_, {}, gpu_mem_total_unit_id_,
gpu_mem_total_global_desc_id_);
} else {
// It's possible for GpuMemTotal ftrace events to be emitted by kworker
diff --git a/src/trace_processor/importers/proto/android_probes_module.cc b/src/trace_processor/importers/proto/android_probes_module.cc
index 141d176..82df69a 100644
--- a/src/trace_processor/importers/proto/android_probes_module.cc
+++ b/src/trace_processor/importers/proto/android_probes_module.cc
@@ -19,6 +19,7 @@
#include "perfetto/base/build_config.h"
#include "perfetto/ext/base/string_writer.h"
#include "perfetto/protozero/scattered_heap_buffer.h"
+#include "src/trace_processor/importers/common/track_tracker.h"
#include "src/trace_processor/importers/proto/android_probes_parser.h"
#include "src/trace_processor/importers/proto/android_probes_tracker.h"
#include "src/trace_processor/timestamped_trace_piece.h"
@@ -76,7 +77,11 @@
using perfetto::protos::pbzero::TracePacket;
AndroidProbesModule::AndroidProbesModule(TraceProcessorContext* context)
- : parser_(context), context_(context) {
+ : parser_(context),
+ context_(context),
+ power_rail_raw_name_id_(context->storage->InternString("raw_name")),
+ power_rail_subsys_name_arg_id_(
+ context->storage->InternString("subsystem_name")) {
RegisterForField(TracePacket::kBatteryFieldNumber, context);
RegisterForField(TracePacket::kPowerRailsFieldNumber, context);
RegisterForField(TracePacket::kAndroidLogFieldNumber, context);
@@ -115,21 +120,28 @@
idx);
continue;
}
- char counter_name[255];
- base::StringWriter writer(counter_name, base::ArraySize(counter_name));
+ base::StackString<255> counter_name("overwritten");
const char* friendly_name = MapToFriendlyPowerRailName(desc.rail_name());
if (friendly_name) {
- writer.AppendStringView("power.rails.");
- writer.AppendStringView(friendly_name);
+ counter_name = base::StackString<255>("power.rails.%s", friendly_name);
} else {
- writer.AppendStringView("power.");
- writer.AppendStringView(desc.rail_name());
- writer.AppendStringView("_uws");
+ counter_name = base::StackString<255>(
+ "power.%s_uws", desc.rail_name().ToStdString().c_str());
}
- AndroidProbesTracker::GetOrCreate(context_)->SetPowerRailNames(
- desc.index(),
- {context_->storage->InternString(desc.rail_name()),
- context_->storage->InternString(writer.GetStringView())});
+ StringId counter_name_id =
+ context_->storage->InternString(counter_name.string_view());
+ TrackId track = context_->track_tracker->InternGlobalCounterTrack(
+ counter_name_id, [this, &desc](ArgsTracker::BoundInserter& inserter) {
+ StringId raw_name = context_->storage->InternString(desc.rail_name());
+ inserter.AddArg(power_rail_raw_name_id_, Variadic::String(raw_name));
+
+ StringId subsys_name =
+ context_->storage->InternString(desc.subsys_name());
+ inserter.AddArg(power_rail_subsys_name_arg_id_,
+ Variadic::String(subsys_name));
+ });
+ AndroidProbesTracker::GetOrCreate(context_)->SetPowerRailTrack(desc.index(),
+ track);
}
// For each energy data message, turn it into its own trace packet
diff --git a/src/trace_processor/importers/proto/android_probes_module.h b/src/trace_processor/importers/proto/android_probes_module.h
index 8b36e37..1a466ad 100644
--- a/src/trace_processor/importers/proto/android_probes_module.h
+++ b/src/trace_processor/importers/proto/android_probes_module.h
@@ -48,6 +48,9 @@
private:
AndroidProbesParser parser_;
TraceProcessorContext* context_ = nullptr;
+
+ const StringId power_rail_raw_name_id_;
+ const StringId power_rail_subsys_name_arg_id_;
};
} // namespace trace_processor
diff --git a/src/trace_processor/importers/proto/android_probes_parser.cc b/src/trace_processor/importers/proto/android_probes_parser.cc
index 7eadba6..bea9336 100644
--- a/src/trace_processor/importers/proto/android_probes_parser.cc
+++ b/src/trace_processor/importers/proto/android_probes_parser.cc
@@ -94,25 +94,16 @@
auto it = evt.energy_data();
protos::pbzero::PowerRails::EnergyData::Decoder desc(*it);
- auto opt_rail_name =
- AndroidProbesTracker::GetOrCreate(context_)->GetPowerRailName(
- desc.index());
- if (opt_rail_name) {
+ auto* tracker = AndroidProbesTracker::GetOrCreate(context_);
+ auto opt_track = tracker->GetPowerRailTrack(desc.index());
+ if (opt_track.has_value()) {
// The tokenization makes sure that this field is always present and
// is equal to the packet's timestamp (as the packet was forged in
// the tokenizer).
PERFETTO_DCHECK(desc.has_timestamp_ms());
PERFETTO_DCHECK(ts / 1000000 == static_cast<int64_t>(desc.timestamp_ms()));
-
- TrackId track = context_->track_tracker->InternGlobalCounterTrack(
- opt_rail_name->friendly);
- context_->event_tracker->PushCounter(
- ts, static_cast<double>(desc.energy()), track,
- [this, opt_rail_name](ArgsTracker::BoundInserter* args_table) {
- args_table->AddArg(
- context_->storage->InternString("power_rails.names.raw"),
- Variadic::String(opt_rail_name->raw));
- });
+ context_->event_tracker->PushCounter(ts, static_cast<double>(desc.energy()),
+ *opt_track);
} else {
context_->storage->IncrementStats(stats::power_rail_unknown_index);
}
diff --git a/src/trace_processor/importers/proto/android_probes_tracker.h b/src/trace_processor/importers/proto/android_probes_tracker.h
index c9a6704..b155cf5 100644
--- a/src/trace_processor/importers/proto/android_probes_tracker.h
+++ b/src/trace_processor/importers/proto/android_probes_tracker.h
@@ -51,27 +51,23 @@
seen_packages_.emplace(std::move(package_name));
}
- struct PowerRailsNames {
- StringId raw;
- StringId friendly;
- };
-
- base::Optional<PowerRailsNames> GetPowerRailName(uint32_t index) {
- if (index >= power_rails_names_str_id_.size())
+ base::Optional<TrackId> GetPowerRailTrack(uint32_t index) {
+ if (index >= power_rail_tracks_.size())
return base::nullopt;
- return power_rails_names_str_id_[index];
+ TrackId track_id = power_rail_tracks_[index];
+ return track_id == kInvalidTrackId ? base::nullopt
+ : base::make_optional(track_id);
}
- void SetPowerRailNames(uint32_t index, PowerRailsNames names) {
- if (power_rails_names_str_id_.size() <= index)
- power_rails_names_str_id_.resize(index + 1);
- power_rails_names_str_id_[index] = names;
+ void SetPowerRailTrack(uint32_t index, TrackId track_id) {
+ if (power_rail_tracks_.size() <= index)
+ power_rail_tracks_.resize(index + 1, kInvalidTrackId);
+ power_rail_tracks_[index] = track_id;
}
private:
std::set<std::string> seen_packages_;
- std::vector<PowerRailsNames> power_rails_names_str_id_;
- std::vector<StringId> power_rails_raw_names_str_id_;
+ std::vector<TrackId> power_rail_tracks_;
};
} // namespace trace_processor
diff --git a/src/trace_processor/importers/proto/gpu_event_parser.cc b/src/trace_processor/importers/proto/gpu_event_parser.cc
index 414a820..c771525 100644
--- a/src/trace_processor/importers/proto/gpu_event_parser.cc
+++ b/src/trace_processor/importers/proto/gpu_event_parser.cc
@@ -745,7 +745,7 @@
if (pid == 0) {
// Pid 0 is used to indicate the global total
track = context_->track_tracker->InternGlobalCounterTrack(
- gpu_mem_total_name_id_, gpu_mem_total_unit_id_,
+ gpu_mem_total_name_id_, {}, gpu_mem_total_unit_id_,
gpu_mem_total_global_desc_id_);
} else {
// Process emitting the packet can be different from the pid in the event.
diff --git a/src/trace_processor/storage/trace_storage.h b/src/trace_processor/storage/trace_storage.h
index 14ecdb3..90380ca 100644
--- a/src/trace_processor/storage/trace_storage.h
+++ b/src/trace_processor/storage/trace_storage.h
@@ -95,10 +95,8 @@
using SnapshotNodeId = tables::MemorySnapshotNodeTable::Id;
-// TODO(lalitm): this is a temporary hack while migrating the counters table and
-// will be removed when the migration is complete.
static const TrackId kInvalidTrackId =
- TrackId(std::numeric_limits<TrackId>::max());
+ TrackId(std::numeric_limits<uint32_t>::max());
enum class RefType {
kRefNoRef = 0,
