lkml.org 
[lkml]   [2019]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/3] perf diff: Support --time filter option
From
Date


On 3/4/2019 11:41 PM, Jiri Olsa wrote:
> On Mon, Mar 04, 2019 at 08:52:38PM +0800, Jin Yao wrote:
>> For better support for perf diff, it would be useful to add --time filter
>> option to diff the samples within given time window.
>>
>> It supports time percent with multipe time ranges. Time string is
>> 'a%/n,b%/m,...' or 'a%-b%,c%-%d,...'.
>>
>> For example:
>>
>> Select the second 10% time slice to diff:
>> perf diff --time 10%/2
>>
>> Select from 0% to 10% time slice to diff:
>> perf diff --time 0%-10%
>>
>> Select the first and the second 10% time slices to diff:
>> perf diff --time 10%/1,10%/2
>>
>> Select from 0% to 10% and 30% to 40% slices to diff:
>> perf diff --time 0%-10%,30%-40%
>>
>> It also supports to analyze samples within given time window as:
>> <start>,<stop>. Times have the format seconds.microseconds. If start
>> is not given (i.e., time string is ',x.y') then analysis starts at
>> the beginning of the file. If stop time is not given (i.e, time
>> string is 'x.y,') then analysis goes to end of file. Time string is
>> 'a1.b1,c1.d1:a2.b2,c2.d2'. Use ':' to separate timestamps for different
>> perf.data files.
>>
>> For example, we get the timestamp information from perf script.
>>
>> perf script -i perf.data.old
>> mgen 13940 [000] 3946.361400: ...
>>
>> perf script -i perf.data
>> mgen 13940 [000] 3971.150589 ...
>>
>> perf diff --time 3946.361400,:3971.150589,
>>
>> It analyzes the perf.data.old from the timestamp 3946.361400 to
>> the end of perf.data.old and analyzes the perf.data from the
>> timestamp 3971.150589 to the end of perf.data.
>>
>> v3:
>> ---
>> Don't parse the time string if --time option is not set.
>> Refactor the code to make it simpler.
>
> it's much clearer now, thanks for doing this, some nits below
>

Thanks!

> SNIP
>
>> +
>> static int __cmd_diff(void)
>> {
>> struct data__file *d;
>> int ret = -EINVAL, i;
>> + char *abstime_ostr, *abstime_tmp;
>> +
>> + abstime_ostr = abstime_str_dup();
>> + abstime_tmp = abstime_ostr;
>
> could be just one line:
>
> abstime_ostr = abstime_tmp = abstime_str_dup();
>
> also please fail if the allocation fails
>

OK, I will fix that.

>>
>> data__for_each_file(i, d) {
>> - d->session = perf_session__new(&d->data, false, &tool);
>> + d->session = perf_session__new(&d->data, false, &pdiff.tool);
>> if (!d->session) {
>> pr_err("Failed to open %s\n", d->data.path);
>> ret = -1;
>> goto out_delete;
>> }
>>
>> + if (pdiff.time_str) {
>> + ret = parse_time_str(d, abstime_ostr, &abstime_tmp);
>> + if (ret < 0)
>> + goto out_delete;
>> + }
>> +
>> ret = perf_session__process_events(d->session);
>> if (ret) {
>> pr_err("Failed to process %s\n", d->data.path);
>> @@ -791,6 +889,9 @@ static int __cmd_diff(void)
>> }
>>
>> perf_evlist__collapse_resort(d->session->evlist);
>> +
>> + if (pdiff.ptime_range)
>> + zfree(&pdiff.ptime_range);
>> }
>>
>> data_process();
>> @@ -802,6 +903,13 @@ static int __cmd_diff(void)
>> }
>>
>> free(data__files);
>> +
>> + if (pdiff.ptime_range)
>> + zfree(&pdiff.ptime_range);
>
> is this zfree needed? you free it in a loop for every data above
>

Yes, we need this zfree. Because if we read the __cmd_diff() in
uiltin-diff.c directly (not only read from this patch), we may see other
"goto out_delete" cases, the zfree in loop may be not called. So we need
an additional zfree checking in error handling.

Thanks
Jin Yao

> jirka
>
>
> SNIP
>

\
 
 \ /
  Last update: 2019-03-05 02:06    [W:0.121 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site