google / perfetto

Performance instrumentation and tracing for Android, Linux and Chrome (read-only mirror of https://android.googlesource.com/platform/external/perfetto/)
https://www.perfetto.dev
Apache License 2.0
2.82k stars 351 forks source link

Optional source file display? #125

Closed gaogaotiantian closed 2 years ago

gaogaotiantian commented 3 years ago

This is a long shot but I think it would be very interesting for perfetto to support source file display when users click on certain slices. It would be useful to quickly debug the code without switching between tabs.

The source files could be stored in protobuf as a TracePacket, with a unique id and it's file type(Java, C++). For each track_event, there could be optional file_id and file_lineno fields, indicating where the original code generated this slice. As both of them are integers, the extra overhead on speed and size won't be much. The file name and line number is actually pretty accessible in C++ with macros.

In the front end, on the callback that currently shows Slice Details, the right half of the panel is unused, which could be used to display source code. I've attached an image of modified catapult trace viewer(which is the front-end of VizTracer now) as an example.

image

Personally, I think this is very helpful when I'm looking at the trace report, and it's not a burden in the front end.

Is this a feature worth considering?

primiano commented 3 years ago

Hmm a couple of thoughts:

  1. Adding source file annotations would bloat quite significantly the size of trace events. Even if we had a working feature I am pretty sure we wouldn't want to add extra bytes to trace events neither in chrome nor in android.
  2. Effectively in chromium most trace event names are unique and you can just copy/paste in codesearch, so often the TraceEvent name is really the key or could make such. It's not 100% collision-proof but works really most of the times.
  3. The part that I am more skeptical about is: how would the UI get access to the file? A browser cannot access random files on the filesystem. The user would have to first point it (for each browsing session) to the source folder. That would reduce the benefit of this by a lot.

All in all the idea is not bad but I feel this is so high-cost (given the complexities above) and low-benefit and I don't think will be worth getting to in O(years)

gaogaotiantian commented 3 years ago
  1. So I double checked the .proto definition, seems like the structure is already in. https://cs.android.com/android/platform/superproject/+/master:external/perfetto/protos/perfetto/trace/track_event/source_location.proto This is exactly the data I mentioned. I don't think this will increase the size of trace events significantly, at least not significantly more than other possible info.
  2. It's true that in most cases, name is a good enough key to search through code. However, you still need to do -> select -> copy -> open your editor -> paste to search -> (skip the wrong ones depends on whether you know/your editor has advanced feature on searching functions) -> find it. Also, it requires that you have the exact local copy of the code executed. For example, when you need to share the trace log to a co-worker, you have to make sure your repos are synced.
  3. The UI won't get access to the file, the file is stored into the trace log as a string. This is a trace-time irrelevant overhead, which should be in kb-mb level, and personally I would tolerate this size of constant overhead on my disk.
  4. The great benefit from this is, when you share the trace log to a colleague, they can see the exact source file you've used, without having to checkout anything, in a single click. Also it would be much easier for anyone to quickly check the source code of a slice.
  5. It took me a week or two to finish the prototype(which I'm using now) on VizTracer + catapult, feel free to check it out at http://www.minkoder.com/viztracer/result.html It probably will take longer in Perfetto as the structures are more complicated and it needs to be polished, but I think this is more a "design decision" than a "complicated task".
  6. The bottom line is, if the user does not like the cost, disabling this feature will be almost exactly the same as it is now, no extra data in trace log or logic on front-end.
primiano commented 3 years ago

source_location.proto is really for things like PostTask in chromium that take a FROM_HERE macro that tells the location in the source code that posted a task (maybe you want something similar though?), is not filled for all events.

The great benefit from this is, when you share the trace log to a colleague, they can see the exact source file you've used, without having to checkout anything, in a single click. Also it would be much easier for anyone to quickly check the source code of a slice.

Yeah but how would the UI know how to lookup the actual source? Unless you pass a full url, like a github-based source URL?

The UI won't get access to the file, the file is stored into the trace log as a string

Oooh wait but then who would take care of storing the full source file into the trace? I mean from at some point you build an executable and run it. How would you inject the source file into the trace at that point?

gaogaotiantian commented 3 years ago

source_location.proto is exactly what I'm looking for in trace packets. The user could optionally use this field to store the source file info for the slice I think? It's already in TrackEvent anyway. Of course if it's conflicting with existing logic, a similar field is fine. I do think this name and it's position represents the exact needs for me though.

Yes I meant storing the full source file(or the selected source file that's interesting) into the trace. The trace creator should be responsible for this, in C++ the Perfetto SDK could utilize __FILE__ and __LINE__ macro(again, this is optional). I'm not familiar with Perfetto trace generating part. I'm thinking from VizTracer point of view, and packing the source file into the trace data is already something I do now.

Actually, a github-based source URL is not a bad idea either, but the sync(making sure the source file matches the executable) is more complicated.

LalitMaganti commented 2 years ago

I believe TrackEvent these days has SourceLocation quite well supported (including offline symbolization) so closing this.