[PATCH 0/5] perf tools: Parent option related fixies for report

[PATCH 0/5] perf tools: Parent option related fixies for report

[Jiri Olsa]

hi,
sending fixies for parent option in report command
pluhs one unrelated oneliner.

thanks,
jirka

Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
---
Jiri Olsa (5):
      perf tools: Remove callchain_cursor_reset call
      perf tools: Fix -x/--exclude-other option for report command
      perf tools: Do not elide parent symbol column
      perf tools: Introduce new -P/--parent-deep report option
      perf tools: Fix perf_session__delete removal for report command

 tools/perf/Documentation/perf-report.txt |  5 +++++
 tools/perf/builtin-diff.c                |  1 -
 tools/perf/builtin-report.c              | 46 ++++++++++++++++++----------------------------
 tools/perf/builtin-top.c                 |  2 --
 tools/perf/util/machine.c                |  6 +++---
 tools/perf/util/symbol.c                 |  1 -
 tools/perf/util/symbol.h                 |  3 ++-
 7 files changed, 28 insertions(+), 36 deletions(-)

[PATCH 1/5] perf tools: Remove callchain_cursor_reset call

[Jiri Olsa]

Removing callchain_cursor_reset call as it is called
in subsequent machine__resolve_callchain_sample
function.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/util/machine.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index b2ecad6..93527af 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1242,8 +1242,6 @@ int machine__resolve_callchain(struct machine *machine,
 {
 	int ret;
 
-	callchain_cursor_reset(&callchain_cursor);
-
 	ret = machine__resolve_callchain_sample(machine, thread,
 						sample->callchain, parent);
 	if (ret)
-- 
1.7.11.7

Re: [PATCH 1/5] perf tools: Remove callchain_cursor_reset call

[David Ahern]

On 6/25/13 5:54 AM, Jiri Olsa wrote:
> Removing callchain_cursor_reset call as it is called
> in subsequent machine__resolve_callchain_sample
> function.
>
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>   tools/perf/util/machine.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index b2ecad6..93527af 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1242,8 +1242,6 @@ int machine__resolve_callchain(struct machine *machine,
>   {
>   	int ret;
>
> -	callchain_cursor_reset(&callchain_cursor);
> -
>   	ret = machine__resolve_callchain_sample(machine, thread,
>   						sample->callchain, parent);
>   	if (ret)
>

I noticed this duplication as well.

Acked-by: David Ahern <dsahern@gmail.com>

[PATCH 2/5] perf tools: Fix -x/--exclude-other option for report command

[Jiri Olsa]

Currently we have symbol_conf.exclude_other being set as true
every time so the -x/--exclude-other has nothing to do. Also
we have no way to see the data with symbol_conf.exclude_other
being false which is useful sometimes.

Fixing it by making symbol_conf.exclude_other false by default.

1) Example without -x option:

  $ perf report -i perf.data.delete -p perf_session__delete -s parent

  +  99.91%  [other]
  +   0.08%  perf_session__delete
  +   0.00%  perf_session__delete_dead_threads
  +   0.00%  perf_session__delete_threads

2) Example with -x option:

  $ ./perf report -i perf.data.delete -p perf_session__delete -s parent -x

  +  96.22%  perf_session__delete
  +   1.89%  perf_session__delete_dead_threads
  +   1.89%  perf_session__delete_threads

In Example 1) we get the sorted out data together with the rest
"[other]". This could help us estimate how much time we spent
in the sorted data.

In Example 2) the total is just the sorted data.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/builtin-diff.c   | 1 -
 tools/perf/builtin-report.c | 3 +--
 tools/perf/builtin-top.c    | 2 --
 tools/perf/util/symbol.c    | 1 -
 4 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index da8f8eb..0aac5f3 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -607,7 +607,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix __maybe_unused)
 		input_new = "perf.data.guest";
 	}
 
-	symbol_conf.exclude_other = false;
 	if (symbol__init() < 0)
 		return -1;
 
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index ca98d34..3662047 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -939,8 +939,7 @@ repeat:
 		 */
 		if (!strstr(sort_order, "parent"))
 			sort_parent.elide = 1;
-	} else
-		symbol_conf.exclude_other = false;
+	}
 
 	if (argc) {
 		/*
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index b03bb9a..a237059 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1139,8 +1139,6 @@ int cmd_top(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (top.evlist == NULL)
 		return -ENOMEM;
 
-	symbol_conf.exclude_other = false;
-
 	argc = parse_options(argc, argv, options, top_usage, 0);
 	if (argc)
 		usage_with_options(top_usage, options);
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 8cf3b54..d5528e1 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -32,7 +32,6 @@ int vmlinux_path__nr_entries;
 char **vmlinux_path;
 
 struct symbol_conf symbol_conf = {
-	.exclude_other	  = true,
 	.use_modules	  = true,
 	.try_vmlinux_path = true,
 	.annotate_src	  = true,
-- 
1.7.11.7

Re: [PATCH 2/5] perf tools: Fix -x/--exclude-other option for report command

[David Ahern]

On 6/25/13 5:54 AM, Jiri Olsa wrote:
> Currently we have symbol_conf.exclude_other being set as true
> every time so the -x/--exclude-other has nothing to do. Also
> we have no way to see the data with symbol_conf.exclude_other
> being false which is useful sometimes.

I'm fine with the change, but wanted to address the "have no way" 
comment: --no-exclude-other should set it to false.

David

[PATCH 3/5] perf tools: Do not elide parent symbol column

[Jiri Olsa]

I found the parent symbol column data interesting even
if there's another sorting enabled. Switching it on.

Previous behaviour:
  $ perf report -i perf.data.delete -p perf_session__delete -x

  +   3.60%  perf  perf               [.] __rb_change_child
  +   1.89%  perf  perf               [.] rb_erase
  +   1.89%  perf  perf               [.] rb_erase
  +   1.83%  perf  perf               [.] free@plt

Current behaviour:
  $ perf report -i perf.data.delete -p perf_session__delete -x

  +   3.60%  perf  perf               [.] __rb_change_child        perf_session__delete
  +   1.89%  perf  perf               [.] rb_erase                 perf_session__delete_dead_threads
  +   1.89%  perf  perf               [.] rb_erase                 perf_session__delete_threads
  +   1.83%  perf  perf               [.] free@plt                 perf_session__delete

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/builtin-report.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 3662047..6ab49da 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -931,14 +931,6 @@ repeat:
 	if (parent_pattern != default_parent_pattern) {
 		if (sort_dimension__add("parent") < 0)
 			goto error;
-
-		/*
-		 * Only show the parent fields if we explicitly
-		 * sort that way. If we only use parent machinery
-		 * for filtering, we don't want it.
-		 */
-		if (!strstr(sort_order, "parent"))
-			sort_parent.elide = 1;
 	}
 
 	if (argc) {
-- 
1.7.11.7

[PATCH 4/5] perf tools: Introduce new -P/--parent-deep report option

[Jiri Olsa]

Introducing new -P/--parent-deep report option. It does the
same as '-p' but it force the deep search of the callchain
and looks for the deepest possible match.

The -p option searches for the first match of the parent
pattern in the callchain.

  $ perf report -i perf.data.delete -p perf_session__delete -s parent

  +  99.51%  [other]
  +   0.46%  perf_session__delete_dead_threads
  +   0.03%  perf_session__delete
  +   0.00%  perf_session__delete_threads

so we got multiple 'different' matches instancies, while
they all belong under perf_session__delete function:

  $ perf report -i perf.data.delete -P perf_session__delete -s parent

  +  99.51%  [other]
  +   0.49%  perf_session__delete

NOTE the 'p' vs 'P' difference in above commands above.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/Documentation/perf-report.txt |  5 +++++
 tools/perf/builtin-report.c              | 12 ++++++++++++
 tools/perf/util/machine.c                |  4 +++-
 tools/perf/util/symbol.h                 |  3 ++-
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 66dab74..90d1566 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -96,6 +96,11 @@ OPTIONS
 	information recorded. The pattern is in the exteneded regex format and
 	defaults to "\^sys_|^do_page_fault", see '--sort parent'.
 
+-P::
+--parent-deep=<regex>::
+	Same as '-p' but it force the deep search of the callchain
+	and looks for the deepest possible match.
+
 -x::
 --exclude-other::
         Only display entries with parent-match.
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 6ab49da..8c2c7ce 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -714,6 +714,16 @@ parse_percent_limit(const struct option *opt, const char *str,
 	return 0;
 }
 
+static int
+parent_deep(const struct option *opt __maybe_unused,
+	    const char *str,
+	    int unset __maybe_unused)
+{
+	parent_pattern = str;
+	symbol_conf.parent_deep = true;
+	return 0;
+}
+
 int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	struct perf_session *session;
@@ -777,6 +787,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		    "Show sample percentage for different cpu modes"),
 	OPT_STRING('p', "parent", &parent_pattern, "regex",
 		   "regex filter to identify parent, see: '--sort parent'"),
+	OPT_CALLBACK('P', "parent-deep", NULL, "regex",
+		     "Enables deep callchain search, implies -p", parent_deep),
 	OPT_BOOLEAN('x', "exclude-other", &symbol_conf.exclude_other,
 		    "Only display entries with parent-match"),
 	OPT_CALLBACK_DEFAULT('g', "call-graph", &report, "output_type,min_percent[,print_limit],call_order",
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 93527af..5ec5580 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1210,7 +1210,9 @@ static int machine__resolve_callchain_sample(struct machine *machine,
 		thread__find_addr_location(thread, machine, cpumode,
 					   MAP__FUNCTION, ip, &al, NULL);
 		if (al.sym != NULL) {
-			if (sort__has_parent && !*parent &&
+			bool more = symbol_conf.parent_deep || !*parent;
+
+			if (sort__has_parent && more &&
 			    symbol__match_parent_regex(al.sym))
 				*parent = al.sym;
 			if (!symbol_conf.use_callchain)
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 5f720dc..6023edb 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -98,7 +98,8 @@ struct symbol_conf {
 			annotate_asm_raw,
 			annotate_src,
 			event_group,
-			demangle;
+			demangle,
+			parent_deep;
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,
-- 
1.7.11.7

Re: [PATCH 4/5] perf tools: Introduce new -P/--parent-deep report option

[Andi Kleen]

On Tue, Jun 25, 2013 at 01:54:12PM +0200, Jiri Olsa wrote:
> Introducing new -P/--parent-deep report option. It does the
> same as '-p' but it force the deep search of the callchain
> and looks for the deepest possible match.

This doesn't make sense to me as an option.

If it's a more correct callgraph it should be default, not 
an option.

A option like "give me correct data" doesn't make sense.

-Andi

Re: [PATCH 4/5] perf tools: Introduce new -P/--parent-deep report option

[Jiri Olsa]

On Tue, Jun 25, 2013 at 08:18:26AM -0700, Andi Kleen wrote:
> On Tue, Jun 25, 2013 at 01:54:12PM +0200, Jiri Olsa wrote:
> > Introducing new -P/--parent-deep report option. It does the
> > same as '-p' but it force the deep search of the callchain
> > and looks for the deepest possible match.
> 
> This doesn't make sense to me as an option.
> 
> If it's a more correct callgraph it should be default, not 
> an option.
> 
> A option like "give me correct data" doesn't make sense.

I thought the '-p' behaviour was for a reason not a bug,
but I can change it to do the deep search and dont add '-P'

jirka

Re: [PATCH 4/5] perf tools: Introduce new -P/--parent-deep report option

[Namhyung Kim]

Hi Jiri,

On Tue, 25 Jun 2013 13:54:12 +0200, Jiri Olsa wrote:
> Introducing new -P/--parent-deep report option. It does the
> same as '-p' but it force the deep search of the callchain
> and looks for the deepest possible match.
>
> The -p option searches for the first match of the parent
> pattern in the callchain.
>
>   $ perf report -i perf.data.delete -p perf_session__delete -s parent
>
>   +  99.51%  [other]
>   +   0.46%  perf_session__delete_dead_threads
>   +   0.03%  perf_session__delete
>   +   0.00%  perf_session__delete_threads
>
> so we got multiple 'different' matches instancies, while
> they all belong under perf_session__delete function:
>
>   $ perf report -i perf.data.delete -P perf_session__delete -s parent
>
>   +  99.51%  [other]
>   +   0.49%  perf_session__delete
>
> NOTE the 'p' vs 'P' difference in above commands above.

I guess using --inverted (-G) option with -p does the job.

Thanks,
Namhyung

[PATCH 5/5] perf tools: Fix perf_session__delete removal for report command

[Jiri Olsa]

There's no point of having out_delete label with perf_session__delete
call within __cmd_report function, because it's called at the
end of the cmd_report function.

The speed up due to commenting out the perf_session__delete
at the end does not seem relevant anymore. Measured speedup
for ~1GB data file with 222466 FORKS events is around 0.5%.

  $ perf report -i perf.data.delete -P perf_session__delete -s parent

  +  99.51%  [other]
  +   0.49%  perf_session__delete

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/builtin-report.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 8c2c7ce..36de70d 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -497,7 +497,7 @@ static int __cmd_report(struct perf_report *rep)
 		ret = perf_session__cpu_bitmap(session, rep->cpu_list,
 					       rep->cpu_bitmap);
 		if (ret)
-			goto out_delete;
+			return ret;
 	}
 
 	if (use_browser <= 0)
@@ -508,11 +508,11 @@ static int __cmd_report(struct perf_report *rep)
 
 	ret = perf_report__setup_sample_type(rep);
 	if (ret)
-		goto out_delete;
+		return ret;
 
 	ret = perf_session__process_events(session, &rep->tool);
 	if (ret)
-		goto out_delete;
+		return ret;
 
 	kernel_map = session->machines.host.vmlinux_maps[MAP__FUNCTION];
 	kernel_kmap = map__kmap(kernel_map);
@@ -547,7 +547,7 @@ static int __cmd_report(struct perf_report *rep)
 
 	if (dump_trace) {
 		perf_session__fprintf_nr_events(session, stdout);
-		goto out_delete;
+		return 0;
 	}
 
 	nr_samples = 0;
@@ -572,7 +572,7 @@ static int __cmd_report(struct perf_report *rep)
 
 	if (nr_samples == 0) {
 		ui__error("The %s file has no samples!\n", session->filename);
-		goto out_delete;
+		return 0;
 	}
 
 	list_for_each_entry(pos, &session->evlist->entries, node)
@@ -598,19 +598,6 @@ static int __cmd_report(struct perf_report *rep)
 	} else
 		perf_evlist__tty_browse_hists(session->evlist, rep, help);
 
-out_delete:
-	/*
-	 * Speed up the exit process, for large files this can
-	 * take quite a while.
-	 *
-	 * XXX Enable this when using valgrind or if we ever
-	 * librarize this command.
-	 *
-	 * Also experiment with obstacks to see how much speed
-	 * up we'll get here.
-	 *
- 	 * perf_session__delete(session);
- 	 */
 	return ret;
 }
 
-- 
1.7.11.7

Re: [PATCH 5/5] perf tools: Fix perf_session__delete removal for report command

[David Ahern]

On 6/25/13 5:54 AM, Jiri Olsa wrote:
> There's no point of having out_delete label with perf_session__delete
> call within __cmd_report function, because it's called at the
> end of the cmd_report function.

Is convenient to have a single return point for a function. Perhaps 
change the label to just out and remove the comment?

David

>
> The speed up due to commenting out the perf_session__delete
> at the end does not seem relevant anymore. Measured speedup
> for ~1GB data file with 222466 FORKS events is around 0.5%.
>
>    $ perf report -i perf.data.delete -P perf_session__delete -s parent
>
>    +  99.51%  [other]
>    +   0.49%  perf_session__delete
>
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Stephane Eranian <eranian@google.com>
> ---
>   tools/perf/builtin-report.c | 23 +++++------------------
>   1 file changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 8c2c7ce..36de70d 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -497,7 +497,7 @@ static int __cmd_report(struct perf_report *rep)
>   		ret = perf_session__cpu_bitmap(session, rep->cpu_list,
>   					       rep->cpu_bitmap);
>   		if (ret)
> -			goto out_delete;
> +			return ret;
>   	}
>
>   	if (use_browser <= 0)
> @@ -508,11 +508,11 @@ static int __cmd_report(struct perf_report *rep)
>
>   	ret = perf_report__setup_sample_type(rep);
>   	if (ret)
> -		goto out_delete;
> +		return ret;
>
>   	ret = perf_session__process_events(session, &rep->tool);
>   	if (ret)
> -		goto out_delete;
> +		return ret;
>
>   	kernel_map = session->machines.host.vmlinux_maps[MAP__FUNCTION];
>   	kernel_kmap = map__kmap(kernel_map);
> @@ -547,7 +547,7 @@ static int __cmd_report(struct perf_report *rep)
>
>   	if (dump_trace) {
>   		perf_session__fprintf_nr_events(session, stdout);
> -		goto out_delete;
> +		return 0;
>   	}
>
>   	nr_samples = 0;
> @@ -572,7 +572,7 @@ static int __cmd_report(struct perf_report *rep)
>
>   	if (nr_samples == 0) {
>   		ui__error("The %s file has no samples!\n", session->filename);
> -		goto out_delete;
> +		return 0;
>   	}
>
>   	list_for_each_entry(pos, &session->evlist->entries, node)
> @@ -598,19 +598,6 @@ static int __cmd_report(struct perf_report *rep)
>   	} else
>   		perf_evlist__tty_browse_hists(session->evlist, rep, help);
>
> -out_delete:
> -	/*
> -	 * Speed up the exit process, for large files this can
> -	 * take quite a while.
> -	 *
> -	 * XXX Enable this when using valgrind or if we ever
> -	 * librarize this command.
> -	 *
> -	 * Also experiment with obstacks to see how much speed
> -	 * up we'll get here.
> -	 *
> - 	 * perf_session__delete(session);
> - 	 */
>   	return ret;
>   }
>
>

[tip:perf/core] perf report: Fix perf_session__delete removal

[tip-bot for Jiri Olsa]

Commit-ID:  d4ae0a6f7c79be64c8f3551dd149189f8c4480eb
Gitweb:     http://git.kernel.org/tip/d4ae0a6f7c79be64c8f3551dd149189f8c4480eb
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Tue, 25 Jun 2013 13:54:13 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 12 Jul 2013 13:46:07 -0300

perf report: Fix perf_session__delete removal

There's no point of having out_delete label with perf_session__delete
call within __cmd_report function, because it's called at the end of the
cmd_report function.

The speed up due to commenting out the perf_session__delete at the end
does not seem relevant anymore. Measured speedup for ~1GB data file with
222466 FORKS events is around 0.5%.

  $ perf report -i perf.data.delete -P perf_session__delete -s parent

  +  99.51%  [other]
  +   0.49%  perf_session__delete

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1372161253-22081-6-git-send-email-jolsa@redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-report.c | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 6ab49da..ee2ca3e 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -497,7 +497,7 @@ static int __cmd_report(struct perf_report *rep)
 		ret = perf_session__cpu_bitmap(session, rep->cpu_list,
 					       rep->cpu_bitmap);
 		if (ret)
-			goto out_delete;
+			return ret;
 	}
 
 	if (use_browser <= 0)
@@ -508,11 +508,11 @@ static int __cmd_report(struct perf_report *rep)
 
 	ret = perf_report__setup_sample_type(rep);
 	if (ret)
-		goto out_delete;
+		return ret;
 
 	ret = perf_session__process_events(session, &rep->tool);
 	if (ret)
-		goto out_delete;
+		return ret;
 
 	kernel_map = session->machines.host.vmlinux_maps[MAP__FUNCTION];
 	kernel_kmap = map__kmap(kernel_map);
@@ -547,7 +547,7 @@ static int __cmd_report(struct perf_report *rep)
 
 	if (dump_trace) {
 		perf_session__fprintf_nr_events(session, stdout);
-		goto out_delete;
+		return 0;
 	}
 
 	nr_samples = 0;
@@ -572,7 +572,7 @@ static int __cmd_report(struct perf_report *rep)
 
 	if (nr_samples == 0) {
 		ui__error("The %s file has no samples!\n", session->filename);
-		goto out_delete;
+		return 0;
 	}
 
 	list_for_each_entry(pos, &session->evlist->entries, node)
@@ -598,19 +598,6 @@ static int __cmd_report(struct perf_report *rep)
 	} else
 		perf_evlist__tty_browse_hists(session->evlist, rep, help);
 
-out_delete:
-	/*
-	 * Speed up the exit process, for large files this can
-	 * take quite a while.
-	 *
-	 * XXX Enable this when using valgrind or if we ever
-	 * librarize this command.
-	 *
-	 * Also experiment with obstacks to see how much speed
-	 * up we'll get here.
-	 *
- 	 * perf_session__delete(session);
- 	 */
 	return ret;
 }