Dev/primes #4298
Dev/primes #4298
Conversation
…race format to Perfetto. There are still a couple of unresolved questions as of this commit.
| optional int32 nanos = 2; | ||
| } | ||
|
|
||
| // Fork of the Primes tracing format (go/primes-ios-tracing-critical-path). |
There was a problem hiding this comment.
Probably shouldn't include a go link here
There was a problem hiding this comment.
Eh, there are a couple scattered throughout the codebase for Googlers to use anyway. I think it's fine? Not opposed to removing it either, though.
There was a problem hiding this comment.
Go links are totally fine to be public.
| message TraceFailure { | ||
| // If non-zero, the offset (from trace start time) at which the trace was | ||
| // considered timed out and failed. | ||
| optional Duration timed_out_after = 1; |
| namespace perfetto::trace_processor::primes { | ||
|
|
||
| // Temporary struct for the primes trace event so we can compile. | ||
| // TODO(leemh): Use the proto instead. Currently compiler can't find |
There was a problem hiding this comment.
Oh yeah, absolutely. I forgot this was still here.
| PrimesTraceTokenizer::~PrimesTraceTokenizer() = default; | ||
|
|
||
| int64_t unify_time(int64_t seconds, int32_t nanos) { | ||
| return seconds * 1000000000LL + nanos; |
There was a problem hiding this comment.
Considering pulling 1000000000LL in to a constant like kNanosToSeconds or changing unify_time to be more descriptive (toNanos?)
| // time. | ||
| if (!start_time_) { | ||
| protozero::Field ts_field = decoder.FindField(kStartTimeFieldNumber); | ||
| if (!ts_field || !ts_field.valid()) { |
There was a problem hiding this comment.
If ts_field.valid() is false, is there a path forward?
There was a problem hiding this comment.
If I'm reading proto_decoder.cc correctly it looks like the field would be null, instead of returning an invalid field. We can probably just remove the || !ts_field.valid().
| auto field_bytes = field.as_bytes(); | ||
| auto start_offset_field = | ||
| protozero::ProtoDecoder(field_bytes.data, field_bytes.size) | ||
| .FindField(kTraceStartOffsetFieldNumber) |
There was a problem hiding this comment.
Should we rename kTraceStartOffsetFieldNumber to make it more clear that it's the field number within TraceEdge?
There was a problem hiding this comment.
Yeah sure - how about kEdgeStartOffsetFieldNumber?
🎨 Perfetto UI Build✅ UI build is ready: https://storage.googleapis.com/perfetto-ci-artifacts/gh-21005753169-1-ui/ui/index.html |
LalitMaganti
left a comment
LalitMaganti
left a comment
There was a problem hiding this comment.
I've left a round of comments but there's some more thorny design problems which need to be addressed before I do a proper review of this.
Main things I'm worried about is the use of maps to keep track of edge id -> slice/track. That seems very suspicious to me as 99.99% of the time, if you're track dimensions are correct, you do not need to do this. And if they're not correct, you'll be breaking the data model and downstream consumers of Perfetto who rely on it.
Specifically, I'm worried about the way you're dealing with slices: I think you might be adding overlapping slices on a single track which is not allowed in the Perfetto data model. You need to use TrackCompressor or put things on different tracks if you need this.
| @@ -0,0 +1 @@ | |||
| // Intentionally empty (crbug.com/998165) | |||
There was a problem hiding this comment.
Why are these checked in? These are autogenerated and live in genfiles folde ronly.
protos/third_party/primes/BUILD.gn
Outdated
| perfetto_proto_library("@TYPE@") { | ||
| sources = [ "primes_tracing.proto" ] | ||
| import_dirs = [ "${perfetto_protobuf_src_dir}" ] | ||
| generate_descriptor = "primes_tracing.descriptor" |
There was a problem hiding this comment.
Why do you need the descirptor? You only need zero generated code.
There was a problem hiding this comment.
Ah yeah, I had no idea what was going on when I did that. Thanks!
| optional int32 nanos = 2; | ||
| } | ||
|
|
||
| // Fork of the Primes tracing format (go/primes-ios-tracing-critical-path). |
There was a problem hiding this comment.
Go links are totally fine to be public.
| PrimesTraceParser::~PrimesTraceParser() = default; | ||
|
|
||
| void PrimesTraceParser::Parse(int64_t ts, TraceBlobView trace_edge) { | ||
| auto proto_decoder = |
There was a problem hiding this comment.
Here and throughout the entire CL:
- why are we using protozero::ProtoDecoder instead of typed proto decoders?
- Why are you doing:
auto edge_decoder =
primespb::TraceEdge_Decoder(field_bytes.data, field_bytes.size);
instead of:
primespb::TraceEdge::Decoder edge_decoder(field_bytes);
which is the style in the rest of the codebase?
There was a problem hiding this comment.
Updated constructor style, and switched to the primes typed proto decoder.
| // executor ID from the parent slice. | ||
| if (sb_decoder.has_executor_id()) { | ||
| UniqueTid utid = context_->process_tracker->GetOrCreateThread( | ||
| (int64_t)sb_decoder.executor_id()); |
There was a problem hiding this comment.
This is not the stylke of casting in the rest of the codebase. Please use static casts.
There was a problem hiding this comment.
Switched to static casts, thanks!
| @@ -0,0 +1,207 @@ | |||
| #include <cstdint> | |||
|
|
|||
| #include "protos/third_party/primes/primes_tracing.pbzero.h" | |||
There was a problem hiding this comment.
Please respect the order of incldes in the rest of the project.
There was a problem hiding this comment.
I think I updated this correctly: lmk if I still borked it. Is there a project style guide anywhere I could look at?
|
|
||
| static constexpr auto kBlueprint = tracks::SliceBlueprint( | ||
| "primes_track", | ||
| tracks::DimensionBlueprints(tracks::kThreadDimensionBlueprint), |
There was a problem hiding this comment.
Please create your own dimension. These are not threads, please don't pretend they are, it makes a lot of things downstream of Perfetto confused if you do.
There was a problem hiding this comment.
Done. This was very illuminating, thank you.
| edge_decoder.id()); | ||
| return; | ||
| } | ||
| slice_name = it->second; |
There was a problem hiding this comment.
This is unnecessary. You can just not pass the name to slice_tracker. It's totally fine to do so.
There was a problem hiding this comment.
Ahhhh, yeah that helps a lot. Appreciate it.
| auto slice_name = context_->storage->InternString(details_decoder.name()); | ||
| parsing_state_.edge_id_to_slice_name_map[edge_decoder.id()] = slice_name; | ||
|
|
||
| std::optional<SliceId> slice_id = |
There was a problem hiding this comment.
Are the slices on the same track actually nested correctly (i.e. in the same way functions are nested?) If not, you cannot just put these things on the same track, they have to use TrackCompressor or be on different tracks.
There was a problem hiding this comment.
I switched to TrackCompressor, like you suggested. I thought about using a combo of (executor_id, thread_id) but TrackCompressor just seems easier. I don't think we really care about what actual thread things were scheduled on for iOS or Android traces... ZaidElkurdi@ do you have thoughts?
| { | ||
| auto it = parsing_state_.edge_id_to_track_id_map.find(edge_decoder.id()); | ||
| if (it == parsing_state_.edge_id_to_track_id_map.end()) { | ||
| PERFETTO_ELOG("Could not find track id for end slice %" PRIu64, |
There was a problem hiding this comment.
ELOGs are not how we report erors in Perfetto. Please use the trace_importer logging apis [1]
[1] http://google3/third_party/perfetto/src/trace_processor/importers/common/import_logs_tracker.h?l=32
There was a problem hiding this comment.
Tried out this API - might have severely misunderstood it just lmk if so.
|
I think this PR is looking closer to what is correct but I imagine there's still some issues. I moved to Really appreciate your help here, Lalit. Still trying to figure out the ropes so I very much appreciate your taking the time here. |
|
Please merge origin/main. It's quite hard to review now because there's a bunch of changes unrelated to your commits polluting the branch now. |
3 participants
b/446912955
Adds support for the Primes (Google internal) trace format to the Perfetto trace processor.