Messages in this thread | | | Date | Mon, 4 Mar 2019 16:41:01 +0100 | From | Jiri Olsa <> | Subject | Re: [PATCH v3 1/3] perf diff: Support --time filter option |
| |
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
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
> > 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
jirka
SNIP
| |