Re: [PATCH v3 1/3] perf diff: Support --time filter option

[Jiri Olsa]

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