Fix perfetto output for using in ff profiler #16
Conversation
* Firefox Profiler requires a "traceEvents" key and that every row has a "name" key (which is not required by the perfetto spec for "E" phases) * WIP - it's not fully functional yet
|
Once finished, this handles |
There was a problem hiding this comment.
Exciting! I've been wanting to get this working for a bunch of time 🎉
Using the previous_state for all events I guess may be buggy when native threads get reused / GvlTracing gets started and stopped, but as you hinted, this will need a overhaul to support the MN scheduling on 3.3 anyway.
I've tried the output on the firefox profiler and it seems... odd -- I can see the events but the lanes at the top are all empty. Did you figure out if there's anything else missing? (or are we just emitting too little information, and that's how firefox profiler renders the info we have right now?)
| case RUBY_INTERNAL_THREAD_EVENT_STARTED: return "started"; | ||
| case RUBY_INTERNAL_THREAD_EVENT_EXITED: return "died"; | ||
| case RUBY_INTERNAL_EVENT_GC_ENTER: return "gc"; | ||
| // TODO: is it possible the thread wasn't running? Might need to save the last state. |
There was a problem hiding this comment.
I don't think this can happen -- from what I've seen, Ruby always triggers GC in the thread that was running, since there's no background GC, so it's always something that the thread does that triggers GC.
| fprintf(output_file, | ||
| // Finish previous duration | ||
| " {\"ph\": \"E\", \"pid\": %"PRId64", \"tid\": %"PRIu64", \"ts\": %f, \"name\": \"%s\"},\n", | ||
| process_id, thread_id, now_microseconds, previous_event_name |
There was a problem hiding this comment.
So if I'm getting this right, the firefox parser wants the event name to be included in both the begin and end phases of a duration event? That's.... kinda weird but ok 🤷♂️
| @@ -195,14 +211,16 @@ static void render_event(const char *event_name) { | |||
| // Important note: We've observed some rendering issues in perfetto if the tid or pid are numbers that are "too big", | |||
| // see https://github.com/ivoanjo/gvl-tracing/pull/4#issuecomment-1196463364 for an example. | |||
|
|
|||
| if (strcmp(event_name, "started_tracing") != 0) { | |||
There was a problem hiding this comment.
Minor: One alternative to avoid doing this every time would be to pass in the previous_event_name as NULL
|
Btw now that Ruby 3.3 support seems to be in good shape, I'd love to get this in as well ;) (no pressure! haha) |
Firefox Profiler requires a "traceEvents" key and that every row has a "name" key (which is not required by the perfetto spec for "E" phases)
The spec doesn't require those https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/edit?pli=1 but the firefox profiler code does
WIP - it's not fully functional yet - it also is more a proof of concept right now since the changes i'll have for Ruby 3.3 will probably refactor away from thead locals, so this code will change as well