Re: [PATCH v3 1/3] perf diff: Support --time filter option
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:
,. 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(>data, false, );
+ d->session = perf_session__new(>data, false, );
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, _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(_range);
}
data_process();
@@ -802,6 +903,13 @@ static int __cmd_diff(void)
}
free(data__files);
+
+ if (pdiff.ptime_range)
+ zfree(_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
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:
> ,. 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(>data, false, );
> + d->session = perf_session__new(>data, false, );
> 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, _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(_range);
> }
>
> data_process();
> @@ -802,6 +903,13 @@ static int __cmd_diff(void)
> }
>
> free(data__files);
> +
> + if (pdiff.ptime_range)
> + zfree(_range);
is this zfree needed? you free it in a loop for every data above
jirka
SNIP
[PATCH v3 1/3] perf diff: Support --time filter option
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:
,. 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.
Signed-off-by: Jin Yao
---
tools/perf/Documentation/perf-diff.txt | 41 ++
tools/perf/builtin-diff.c | 136 +
2 files changed, 164 insertions(+), 13 deletions(-)
diff --git a/tools/perf/Documentation/perf-diff.txt
b/tools/perf/Documentation/perf-diff.txt
index a79c84a..5bfe876 100644
--- a/tools/perf/Documentation/perf-diff.txt
+++ b/tools/perf/Documentation/perf-diff.txt
@@ -118,6 +118,47 @@ OPTIONS
sum of shown entries will be always 100%. "absolute" means it retains
the original value before and after the filter is applied.
+--time::
+ Analyze 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:
+ ,. 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.
+
COMPARISON
--
The comparison is governed by the baseline file. The baseline perf.data
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 58fe0e8..2c49dda 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -19,12 +19,21 @@
#include "util/util.h"
#include "util/data.h"
#include "util/config.h"
+#include "util/time-utils.h"
#include
#include
#include
#include
+struct perf_diff {
+ struct perf_tool tool;
+ const char *time_str;
+ struct perf_time_interval *ptime_range;
+ int range_size;
+ int range_num;
+};
+
/* Diff command specific HPP columns. */
enum {
PERF_HPP_DIFF__BASELINE,
@@ -323,16 +332,22 @@ static int formula_fprintf(struct hist_entry *he, struct
hist_entry *pair,
return -1;
}
-static int diff__process_sample_event(struct perf_tool *tool __maybe_unused,
+static int diff__process_sample_event(struct perf_tool *tool,
union perf_event *event,
struct perf_sample *sample,
struct perf_evsel *evsel,
struct machine *machine)
{
+ struct perf_diff *pdiff = container_of(tool,
