Messages in this thread | | | Subject | Re: [PATCH] perf data: Add JSON export | From | Nicholas Fraser <> | Date | Tue, 6 Apr 2021 05:31:51 -0400 |
| |
Hi Jiri,
Thanks again for the review. I've fixed the issues you mentioned. Some notes below:
On 2021-04-01 8:15 a.m., Jiri Olsa wrote: > I recall you did not add support for walltime clock, > don't you need it to sync with other events?
Not necessarily. As long as the perf recording and the GPU trace are made from the same clock, the events should line up; the real time doesn't matter. I've added it anyway though since it's good to have. It now outputs "clockid", "clock-time" and "real-time" in the headers if clock info was included in the recording.
On 2021-04-01 9:18 a.m., Jiri Olsa wrote: > also I will not push hard for test, becase we don't have any for CTF ;-) > but if you could think of any, that'd be great
I'm not sure what kind of tests would be useful. We could include a small perf.data file and its JSON output and verify that they match. This is probably pointless because almost any change to the JSON code would be expected to change the output so we'd just be resetting the test on each change.
> I was wondering how to make this \t mess more readable, > how about you define function like output_json: > > output_json(FILE, level, field, format, ...); > > and use it: > > output_json(c->out, 3, "data-offset", "PRIu64", header->data_offset); > output_json(c->out, 3, "data-size", "PRIu64", header->data_size); > output_json(c->out, 3, "feat-offset", PRIu64, header->feat_offset); > > similar way as we do for pr_debug -> eprintf
I've cleaned up the output with some additional helper functions. This is essentially turning into its own mini JSON encoder which I was hoping to avoid but I suppose it's necessary to make the code maintainable.
I'll be sending a new patch shortly.
Nick
| |