Messages in this thread | | | Subject | Re: [PATCH v3 1/3] perf diff: Support --time filter option | From | "Jin, Yao" <> | Date | Tue, 5 Mar 2019 09:06:04 +0800 |
| |
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 >
| |