Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310972 > unrolled thread

[RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

Back to article view | Back to linux.kernel

Contents

  [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    [PATCH 16/17] perf hists: Support decaying in hierarchy mode Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    [PATCH 13/17] perf hists browser: Align column header in hierarchy mode Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    [PATCH 15/17] perf report: Add --hierarchy option Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    [PATCH 01/17] perf hists: Basic support of hierarchical report view Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
      Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Jiri Olsa <jolsa@redhat.com> - 2016-01-17 17:20 +0100
        Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Namhyung Kim <namhyung@kernel.org> - 2016-01-19 12:00 +0100
          Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-19 18:00 +0100
            Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Jiri Olsa <jolsa@redhat.com> - 2016-01-20 18:10 +0100
              Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-20 18:20 +0100
              Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Namhyung Kim <namhyung@kernel.org> - 2016-01-21 05:10 +0100
      Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Jiri Olsa <jolsa@redhat.com> - 2016-01-21 11:50 +0100
        Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Namhyung Kim <namhyung@kernel.org> - 2016-01-21 14:00 +0100
          Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Jiri Olsa <jolsa@redhat.com> - 2016-01-21 14:40 +0100
            Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-21 15:10 +0100
              Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Namhyung Kim <namhyung@kernel.org> - 2016-01-22 11:50 +0100
            Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Namhyung Kim <namhyung@kernel.org> - 2016-01-22 11:50 +0100
              Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Jiri Olsa <jolsa@redhat.com> - 2016-01-22 12:40 +0100
      Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Jiri Olsa <jolsa@redhat.com> - 2016-01-21 12:40 +0100
        Re: [PATCH 01/17] perf hists: Basic support of hierarchical report  view Namhyung Kim <namhyung@kernel.org> - 2016-01-21 14:10 +0100
    [PATCH 08/17] perf hists browser: Fix context menu item Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
      Re: [PATCH 08/17] perf hists browser: Fix context menu item Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-21 02:00 +0100
        Re: [PATCH 08/17] perf hists browser: Fix context menu item Namhyung Kim <namhyung@kernel.org> - 2016-01-21 05:10 +0100
          Re: [PATCH 08/17] perf hists browser: Fix context menu item Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-22 01:00 +0100
            Re: [PATCH 08/17] perf hists browser: Fix context menu item Namhyung Kim <namhyung@kernel.org> - 2016-01-22 12:10 +0100
      Dynamicly add/remove sort keys was: Re: [PATCH 08/17] perf hists  browser: Fix context menu item Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-22 15:40 +0100
    [PATCH 04/17] perf hists: Cleanup filtering functions Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
      Re: [PATCH 04/17] perf hists: Cleanup filtering functions Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-19 21:40 +0100
        [PATCH v2 04.1/17] perf hists: Remove parent filter check in DSO filter function Namhyung Kim <namhyung@kernel.org> - 2016-01-20 02:30 +0100
          [PATCH v2 04.2/17] perf hists: Cleanup filtering functions Namhyung Kim <namhyung@kernel.org> - 2016-01-20 02:30 +0100
            Re: [PATCH v2 04.2/17] perf hists: Cleanup filtering functions Jiri Olsa <jolsa@redhat.com> - 2016-01-21 13:10 +0100
          Re: [PATCH v2 04.1/17] perf hists: Remove parent filter check in DSO  filter function Jiri Olsa <jolsa@redhat.com> - 2016-01-21 13:10 +0100
    [PATCH 02/17] perf hists: Resort hist entries with hierarchy Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
      Re: [PATCH 02/17] perf hists: Resort hist entries with hierarchy Jiri Olsa <jolsa@redhat.com> - 2016-01-21 12:50 +0100
        Re: [PATCH 02/17] perf hists: Resort hist entries with hierarchy Namhyung Kim <namhyung@kernel.org> - 2016-01-21 14:10 +0100
    [PATCH 10/17] perf hists browser: Support collapsing/expanding whole entries in hierarchy Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    [PATCH 07/17] perf ui/stdio: Align column header for hierarchy output Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
      Re: [PATCH 07/17] perf ui/stdio: Align column header for hierarchy  output Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-20 23:50 +0100
        Re: [PATCH 07/17] perf ui/stdio: Align column header for hierarchy  output Namhyung Kim <namhyung@kernel.org> - 2016-01-21 05:10 +0100
    [PATCH 12/17] perf hists browser: Implement hierarchy output Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    [PATCH 05/17] perf hists: Support filtering in hierarchy mode Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    [PATCH 11/17] perf hists browser: Factor out hist_browser__show_callchain() Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    [PATCH 06/17] perf ui/stdio: Implement hierarchy output mode Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    [PATCH 03/17] perf hists: Add helper functions for hierarchy mode Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
      Re: [PATCH 03/17] perf hists: Add helper functions for hierarchy mode Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-20 23:20 +0100
        Re: [PATCH 03/17] perf hists: Add helper functions for hierarchy mode Namhyung Kim <namhyung@kernel.org> - 2016-01-21 05:00 +0100
          [PATCH v2 03/17] perf hists: Add helper functions for hierarchy mode Namhyung Kim <namhyung@kernel.org> - 2016-01-21 05:20 +0100
            Re: [PATCH v2 03/17] perf hists: Add helper functions for hierarchy  mode Namhyung Kim <namhyung@kernel.org> - 2016-01-21 14:10 +0100
    [PATCH 09/17] perf hists browser: Count number of hierarchy entries Namhyung Kim <namhyung@kernel.org> - 2016-01-16 17:10 +0100
    Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2) Pekka Enberg <penberg@kernel.org> - 2016-01-17 11:30 +0100
      Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-19 11:50 +0100
    Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Andi Kleen <andi@firstfloor.org> - 2016-01-17 20:40 +0100
      Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-19 11:50 +0100
        Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-19 22:10 +0100
          Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-19 22:10 +0100
        Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Andi Kleen <andi@firstfloor.org> - 2016-01-19 23:20 +0100
          Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-19 23:30 +0100
            Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-20 02:00 +0100
              Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-20 02:40 +0100
                Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Andi Kleen <andi@firstfloor.org> - 2016-01-20 02:50 +0100
              Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Andi Kleen <andi@firstfloor.org> - 2016-01-20 03:00 +0100
              Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-20 14:40 +0100
    Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-19 21:10 +0100
    Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-19 22:00 +0100
      Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-20 01:40 +0100
        Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Andi Kleen <andi@firstfloor.org> - 2016-01-20 06:30 +0100
        Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Taeung Song <taeung.dev@gmail.com> - 2016-01-20 09:00 +0100
          Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-20 16:10 +0100
            Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Taeung Song <taeung.dev@gmail.com> - 2016-01-20 17:40 +0100
              Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-21 05:20 +0100
                Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Taeung Song <taeung.dev@gmail.com> - 2016-01-21 06:00 +0100
        Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-20 14:40 +0100
          Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-20 16:10 +0100
            Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-20 16:30 +0100
              Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-20 16:30 +0100
    Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-01-19 22:00 +0100
      Re: [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view  (v2) Namhyung Kim <namhyung@kernel.org> - 2016-01-20 01:20 +0100

Page 1 of 4  [1] 2 3 4  Next page →

#1310972 — [RFC/PATCHSET 00/17] perf tools: Add support for hierachy view (v2)

Hello,

This is v2 attempt of my earlier patchset [1].  This patchset
implements a new feature that collects hist entries in a hierachical
manner.  That means lower-level entries belong to an upper-level
entry.  The entry hierachy is built on the sort keys given, so users
can set it whatever they want.  It only shows top-level entries first,
and user can expand/collapse it dynamically.

This time I implemented it for every output browser including TUI.
A screenshot on TUI looks like below:

For normal output:

  $ perf report --tui
  Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
    Overhead  Command        Shared Object         Symbol
  ------------------------------------------------------------------------
  -    7.57%  swapper        [kernel.vmlinux]      [k] intel_idle
       intel_idle
       cpuidle_enter_state
       cpuidle_enter
       call_cpuidle
     + cpu_startup_entry
  +    1.16   firefox        firefox               [.] 0x00000000000019433
  +    0.97%  firefox        libpthread-2.22.so    [.] pthread_mutex_lock
  ...


With hierarchy view,

  $ perf report --tui --hierarchy
  Samples: 3K of event 'cycles:pp', Event count (approx.): 1695979674
   Overhead        Command / Shared Object / Symbol
  -------------------------------------------------------------------
  +  76.30%        firefox
  -   9.95%        swapper
     -   9.51%        [kernel.vmlinux]
        -   7.57         [k] intel_idle
	     intel_idle
	     cpuidle_enter_state
	     cpuidle_enter
	     call_cpuidle
	   + cpu_startup_entry
	+   0.15%        [k] __schedule
	+   0.12%        [k] menu_select
	...
     +   0.34%        [sdhci]
     +   0.06%        [e1000e]
     ...
 +    5.65%        Xorg
 +    5.42%        Socket Thread
 ...

As you can see, overhead of an upper level entry is the sum of
overhead of lower level entries.  The entries are aligned by its order
of matching sort keys.

This is available from 'perf/hierarchy-v2' branch in my tree:

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung-perf.git


Any comments are welcome, thanks!
Namhyung


Cc: Don Zickus <dzickus@redhat.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Moinuddin Quadri <moin18@gmail.com>


[1] https://lkml.org/lkml/2013/5/21/24


Namhyung Kim (17):
  perf hists: Basic support of hierarchical report view
  perf hists: Resort hist entries with hierarchy
  perf hists: Add helper functions for hierarchy mode
  perf hists: Cleanup filtering functions
  perf hists: Support filtering in hierarchy mode
  perf ui/stdio: Implement hierarchy output mode
  perf ui/stdio: Align column header for hierarchy output
  perf hists browser: Fix context menu item
  perf hists browser: Count number of hierarchy entries
  perf hists browser: Support collapsing/expanding whole entries in
    hierarchy
  perf hists browser: Factor out hist_browser__show_callchain()
  perf hists browser: Implement hierarchy output
  perf hists browser: Align column header in hierarchy mode
  perf ui/gtk: Implement hierarchy output mode
  perf report: Add --hierarchy option
  perf hists: Support decaying in hierarchy mode
  perf top: Add --hierarchy option

 tools/perf/Documentation/perf-report.txt |   3 +
 tools/perf/Documentation/perf-top.txt    |   3 +
 tools/perf/Documentation/tips.txt        |   1 +
 tools/perf/builtin-report.c              |  17 +
 tools/perf/builtin-top.c                 |  15 +
 tools/perf/ui/browsers/hists.c           | 590 ++++++++++++++++++++++++++-----
 tools/perf/ui/gtk/hists.c                | 161 ++++++++-
 tools/perf/ui/hist.c                     |  14 +
 tools/perf/ui/stdio/hist.c               | 182 +++++++++-
 tools/perf/util/hist.c                   | 470 ++++++++++++++++++++----
 tools/perf/util/hist.h                   |  11 +
 tools/perf/util/sort.c                   | 116 ++++++
 tools/perf/util/sort.h                   |  16 +-
 tools/perf/util/symbol.h                 |   3 +-
 14 files changed, 1437 insertions(+), 165 deletions(-)

-- 
2.6.4

[toc] | [next] | [standalone]

#1310974 — [PATCH 16/17] perf hists: Support decaying in hierarchy mode

In the hierarchy mode, hist entries should decay their children too.
Also update hists__delete_entry() to be able to free child entries.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index a0808f73e0d9..979cd6091034 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -243,6 +243,8 @@ static void he_stat__decay(struct he_stat *he_stat)
 	/* XXX need decay for weight too? */
 }
 
+static void hists__delete_entry(struct hists *hists, struct hist_entry *he);
+
 static bool hists__decay_entry(struct hists *hists, struct hist_entry *he)
 {
 	u64 prev_period = he->stat.period;
@@ -258,21 +260,45 @@ static bool hists__decay_entry(struct hists *hists, struct hist_entry *he)
 
 	diff = prev_period - he->stat.period;
 
-	hists->stats.total_period -= diff;
-	if (!he->filtered)
-		hists->stats.total_non_filtered_period -= diff;
+	if (!he->depth) {
+		hists->stats.total_period -= diff;
+		if (!he->filtered)
+			hists->stats.total_non_filtered_period -= diff;
+	}
+
+	if (!he->leaf) {
+		struct hist_entry *child;
+		struct rb_node *node = rb_first(&he->hroot_out);
+		while (node) {
+			child = rb_entry(node, struct hist_entry, rb_node);
+			node = rb_next(node);
+
+			if (hists__decay_entry(hists, child))
+				hists__delete_entry(hists, child);
+		}
+	}
 
 	return he->stat.period == 0;
 }
 
 static void hists__delete_entry(struct hists *hists, struct hist_entry *he)
 {
-	rb_erase(&he->rb_node, &hists->entries);
+	struct rb_root *root_in;
+	struct rb_root *root_out;
 
-	if (sort__need_collapse)
-		rb_erase(&he->rb_node_in, &hists->entries_collapsed);
-	else
-		rb_erase(&he->rb_node_in, hists->entries_in);
+	if (he->parent_he) {
+		root_in  = &he->parent_he->hroot_in;
+		root_out = &he->parent_he->hroot_out;
+	} else {
+		if (sort__need_collapse)
+			root_in = &hists->entries_collapsed;
+		else
+			root_in = hists->entries_in;
+		root_out = &hists->entries;
+	}
+
+	rb_erase(&he->rb_node_in, root_in);
+	rb_erase(&he->rb_node, root_out);
 
 	--hists->nr_entries;
 	if (!he->filtered)
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

#1310975 — [PATCH 13/17] perf hists browser: Align column header in hierarchy mode

Like in stdio, fit column header to hierarchy output.  Merge column
headers with "/" as a separator.

   Overhead        Command / Shared Object / Symbol
  ...
  +   0.09%        dwm
  +   0.06%        emacs
  -   0.05%        perf
     -   0.05%        [kernel.vmlinux]
        +   0.03%        [k] memcpy_orig
        +   0.01%        [k] unmap_single_vma
        +   0.01%        [k] smp_call_function_single
        +   0.00%        [k] native_irq_return_iret
        +   0.00%        [k] arch_trigger_all_cpu_backtrace_handler
        +   0.00%        [k] native_write_msr_safe

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/ui/browsers/hists.c | 69 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index ded026e213cc..11728249f70a 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1412,11 +1412,78 @@ static int hists_browser__scnprintf_headers(struct hist_browser *browser, char *
 	return ret;
 }
 
+static int hists_browser__scnprintf_hierarchy_headers(struct hist_browser *browser, char *buf, size_t size)
+{
+	struct hists *hists = browser->hists;
+	struct perf_hpp dummy_hpp = {
+		.buf    = buf,
+		.size   = size,
+	};
+	struct perf_hpp_fmt *fmt;
+	size_t ret = 0;
+	int column = 0;
+	int nr_sort_keys = perf_hpp__count_sort_keys();
+	bool first = true;
+
+	ret = scnprintf(buf, size, " ");
+	if (advance_hpp_check(&dummy_hpp, ret))
+		return ret;
+
+	perf_hpp__for_each_format(fmt) {
+		if (column++ < browser->b.horiz_scroll)
+			continue;
+
+		if (perf_hpp__is_sort_entry(fmt) || perf_hpp__is_dynamic_entry(fmt))
+			break;
+
+		ret = fmt->header(fmt, &dummy_hpp, hists_to_evsel(hists));
+		if (advance_hpp_check(&dummy_hpp, ret))
+			break;
+
+		ret = scnprintf(dummy_hpp.buf, dummy_hpp.size, "  ");
+		if (advance_hpp_check(&dummy_hpp, ret))
+			break;
+	}
+
+	ret = scnprintf(dummy_hpp.buf, dummy_hpp.size, "%*s",
+			(nr_sort_keys - 1) * HIERARCHY_INDENT, "");
+	if (advance_hpp_check(&dummy_hpp, ret))
+		return ret;
+
+	perf_hpp__for_each_format(fmt) {
+		if (!perf_hpp__is_sort_entry(fmt) && !perf_hpp__is_dynamic_entry(fmt))
+			continue;
+
+		if (first) {
+			first = false;
+		} else {
+			ret = scnprintf(dummy_hpp.buf, dummy_hpp.size, " / ");
+			if (advance_hpp_check(&dummy_hpp, ret))
+				break;
+		}
+
+		ret = fmt->header(fmt, &dummy_hpp, hists_to_evsel(hists));
+		dummy_hpp.buf[ret] = '\0';
+		rtrim(dummy_hpp.buf);
+
+		ret = strlen(dummy_hpp.buf);
+		if (advance_hpp_check(&dummy_hpp, ret))
+			break;
+	}
+
+	return ret;
+}
+
 static void hist_browser__show_headers(struct hist_browser *browser)
 {
 	char headers[1024];
 
-	hists_browser__scnprintf_headers(browser, headers, sizeof(headers));
+	if (symbol_conf.report_hierarchy)
+		hists_browser__scnprintf_hierarchy_headers(browser, headers,
+							   sizeof(headers));
+	else
+		hists_browser__scnprintf_headers(browser, headers,
+						 sizeof(headers));
 	ui_browser__gotorc(&browser->b, 0, 0);
 	ui_browser__set_color(&browser->b, HE_COLORSET_ROOT);
 	ui_browser__write_nstring(&browser->b, headers, browser->b.width + 1);
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

#1310976 — [PATCH 15/17] perf report: Add --hierarchy option

The --hierarchy option is to show output in hierarchy mode.  It extends
folding/unfolding in the TUI and GTK browsers to support sort items as
well as callchains.  Users can toggle the items to see the performance
result at wanted level.

  $ perf report --hierarchy --tui
   Overhead        Command / Shared Object / Symbol
  +  32.96%        gnome-shell
  -  15.11%        swapper
     -  14.97%        [kernel.vmlinux]
            6.82%        [k] intel_idle
	    0.66%        [k] menu_select
	    0.43%        [k] __hrtimer_start_range_ns
  ...

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-report.txt |  3 +++
 tools/perf/Documentation/tips.txt        |  1 +
 tools/perf/builtin-report.c              | 17 +++++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index 8a301f6afb37..d2b4d6e15622 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -398,6 +398,9 @@ include::itrace.txt[]
 --raw-trace::
 	When displaying traceevent output, do not use print fmt or plugins.
 
+--hierarchy::
+	Enable hierarchical output.
+
 include::callchain-overhead-calculation.txt[]
 
 SEE ALSO
diff --git a/tools/perf/Documentation/tips.txt b/tools/perf/Documentation/tips.txt
index e0ce9573b79b..5950b5a24efd 100644
--- a/tools/perf/Documentation/tips.txt
+++ b/tools/perf/Documentation/tips.txt
@@ -27,3 +27,4 @@ Skip collecing build-id when recording: perf record -B
 To change sampling frequency to 100 Hz: perf record -F 100
 See assembly instructions with percentage: perf annotate <symbol>
 If you prefer Intel style assembly, try: perf annotate -M intel
+For hierarchical output, try: perf report --hierarchy
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 2bf537f190a0..fd26db0f4c74 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -798,6 +798,8 @@ int cmd_report(int argc, const char **argv, const char *prefix __maybe_unused)
 		    "only show processor socket that match with this filter"),
 	OPT_BOOLEAN(0, "raw-trace", &symbol_conf.raw_trace,
 		    "Show raw trace event output (do not use print fmt or plugins)"),
+	OPT_BOOLEAN(0, "hierarchy", &symbol_conf.report_hierarchy,
+		    "Show entries in a hierarchy"),
 	OPT_END()
 	};
 	struct perf_data_file file = {
@@ -907,6 +909,21 @@ repeat:
 		symbol_conf.cumulate_callchain = false;
 	}
 
+	if (symbol_conf.report_hierarchy) {
+		/* disable incompatible options */
+		symbol_conf.event_group = false;
+		symbol_conf.cumulate_callchain = false;
+
+		if (field_order) {
+			pr_err("Error: --hierarchy and --fields options cannot be used together\n");
+			parse_options_usage(report_usage, options, "F", 1);
+			parse_options_usage(NULL, options, "hierarchy", 0);
+			goto error;
+		}
+
+		sort__need_collapse = true;
+	}
+
 	if (setup_sorting(session->evlist) < 0) {
 		if (sort_order)
 			parse_options_usage(report_usage, options, "s", 1);
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

#1310977 — [PATCH 01/17] perf hists: Basic support of hierarchical report view

In the hierarchical view, entries will be grouped and sorted on the
first key, and then second key, and so on.  Add he->hroot_{in,out} field
to keep lower level entries. Actually this can be shared with callchain
sorted_root since the hroots are only used by non-leaf entries and
callchain is only used by leaf entries.

It also adds parent_he and depth fields which can be used by browsers.

This patch only implements collapsing part which creates internal
entries for each sort key.  These need to be sorted by output_sort stage
and to be displayed properly in the later patch(es).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/hist.c   | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
 tools/perf/util/sort.h   | 13 ++++++-
 tools/perf/util/symbol.h |  3 +-
 3 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index c226303e3da0..70293eb70430 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -1007,6 +1007,95 @@ void hist_entry__delete(struct hist_entry *he)
  * collapse the histogram
  */
 
+static void hists__apply_filters(struct hists *hists, struct hist_entry *he);
+
+static struct hist_entry *hierarchy_insert_entry(struct hists *hists,
+						 struct rb_root *root,
+						 struct hist_entry *he,
+						 struct perf_hpp_fmt *fmt)
+{
+	struct rb_node **p = &root->rb_node;
+	struct rb_node *parent = NULL;
+	struct hist_entry *iter, *new;
+	int64_t cmp;
+
+	while (*p != NULL) {
+		parent = *p;
+		iter = rb_entry(parent, struct hist_entry, rb_node_in);
+
+		cmp = fmt->collapse(fmt, iter, he);
+		if (!cmp) {
+			he_stat__add_stat(&iter->stat, &he->stat);
+			return iter;
+		}
+
+		if (cmp < 0)
+			p = &parent->rb_left;
+		else
+			p = &parent->rb_right;
+	}
+
+	new = hist_entry__new(he, true);
+	if (new == NULL)
+		return false;
+
+	hists__apply_filters(hists, new);
+	hists->nr_entries++;
+
+	/* save related format for output */
+	new->fmt = fmt;
+
+	/* it's now passed to 'new' */
+	he->trace_output = NULL;
+
+	rb_link_node(&new->rb_node_in, parent, p);
+	rb_insert_color(&new->rb_node_in, root);
+	return new;
+}
+
+static bool hists__hierarchy_insert_entry(struct hists *hists,
+					  struct rb_root *root,
+					  struct hist_entry *he)
+{
+	struct perf_hpp_fmt *fmt;
+	struct hist_entry *new = NULL;
+	struct hist_entry *parent = NULL;
+	int depth = 0;
+
+	perf_hpp__for_each_sort_list(fmt) {
+		if (!perf_hpp__is_sort_entry(fmt) &&
+		    !perf_hpp__is_dynamic_entry(fmt))
+			continue;
+
+		/* insert copy of 'he' for each fmt into the hierarchy */
+		new = hierarchy_insert_entry(hists, root, he, fmt);
+		if (new == NULL)
+			break;
+
+		root = &new->hroot_in;
+		new->parent_he = parent;
+		new->depth = depth++;
+		parent = new;
+	}
+
+	if (new) {
+		new->leaf = true;
+
+		if (symbol_conf.use_callchain) {
+			callchain_cursor_reset(&callchain_cursor);
+			callchain_merge(&callchain_cursor,
+					new->callchain,
+					he->callchain);
+		}
+	}
+
+	/* 'he' is no longer used */
+	hist_entry__delete(he);
+
+	/* it already applied filters */
+	return false;
+}
+
 bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 				  struct rb_root *root, struct hist_entry *he)
 {
@@ -1015,6 +1104,9 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 	struct hist_entry *iter;
 	int64_t cmp;
 
+	if (symbol_conf.report_hierarchy)
+		return hists__hierarchy_insert_entry(hists, root, he);
+
 	while (*p != NULL) {
 		parent = *p;
 		iter = rb_entry(parent, struct hist_entry, rb_node_in);
@@ -1042,6 +1134,7 @@ bool hists__collapse_insert_entry(struct hists *hists __maybe_unused,
 			p = &(*p)->rb_right;
 	}
 	hists->nr_entries++;
+	he->leaf = true;
 
 	rb_link_node(&he->rb_node_in, parent, p);
 	rb_insert_color(&he->rb_node_in, root);
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 687bbb124428..15a75d44de91 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -94,9 +94,11 @@ struct hist_entry {
 	s32			socket;
 	s32			cpu;
 	u8			cpumode;
+	u8			depth;
 
 	/* We are added by hists__add_dummy_entry. */
 	bool			dummy;
+	bool			leaf;
 
 	char			level;
 	u8			filtered;
@@ -118,13 +120,22 @@ struct hist_entry {
 	char			*srcline;
 	char			*srcfile;
 	struct symbol		*parent;
-	struct rb_root		sorted_chain;
 	struct branch_info	*branch_info;
 	struct hists		*hists;
 	struct mem_info		*mem_info;
 	void			*raw_data;
 	u32			raw_size;
 	void			*trace_output;
+	struct perf_hpp_fmt	*fmt;
+	struct hist_entry	*parent_he;
+	union {
+		/* this is for hierarchical entry structure */
+		struct {
+			struct rb_root	hroot_in;
+			struct rb_root  hroot_out;
+		};				/* non-leaf entries */
+		struct rb_root	sorted_chain;	/* leaf entry has callchains */
+	};
 	struct callchain_root	callchain[0]; /* must be last member */
 };
 
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index ccd1caa40e11..a937053a0ae0 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -110,7 +110,8 @@ struct symbol_conf {
 			has_filter,
 			show_ref_callgraph,
 			hide_unresolved,
-			raw_trace;
+			raw_trace,
+			report_hierarchy;
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,
-- 
2.6.4

[toc] | [prev] | [next] | [standalone]

#1311165 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:

SNIP

>  	char			*srcfile;
>  	struct symbol		*parent;
> -	struct rb_root		sorted_chain;
>  	struct branch_info	*branch_info;
>  	struct hists		*hists;
>  	struct mem_info		*mem_info;
>  	void			*raw_data;
>  	u32			raw_size;
>  	void			*trace_output;
> +	struct perf_hpp_fmt	*fmt;
> +	struct hist_entry	*parent_he;
> +	union {
> +		/* this is for hierarchical entry structure */
> +		struct {
> +			struct rb_root	hroot_in;
> +			struct rb_root  hroot_out;
> +		};				/* non-leaf entries */
> +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> +	};

looks like cool feature!

could we have the hist_entry storage little more generic?
and maybe dynamically allocated?

I'm doing the same thing for the c2c stuff, like having
for each hist_entry its own 'struct hists' object, which
records data related to parent hist_entry

maybe we could strip the 'hists' object to some bare minimum
which is needed for store/sort/stat and display entries
in hists_browser ;-)

I'm preparing RFC patchset in my perf/c2c branch, if you want
to take a look

however, as I said above, for my own sake it all boils down
to have 'hists' object within hist_entry, so I can use it
in the UI code easily

FYI I also added support for hists object's own sort/output
format lists.. which I'll curve out and send for review soon

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1312109 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

Hi Jiri,

On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> >  	char			*srcfile;
> >  	struct symbol		*parent;
> > -	struct rb_root		sorted_chain;
> >  	struct branch_info	*branch_info;
> >  	struct hists		*hists;
> >  	struct mem_info		*mem_info;
> >  	void			*raw_data;
> >  	u32			raw_size;
> >  	void			*trace_output;
> > +	struct perf_hpp_fmt	*fmt;
> > +	struct hist_entry	*parent_he;
> > +	union {
> > +		/* this is for hierarchical entry structure */
> > +		struct {
> > +			struct rb_root	hroot_in;
> > +			struct rb_root  hroot_out;
> > +		};				/* non-leaf entries */
> > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > +	};
> 
> looks like cool feature!

Thanks!

> 
> could we have the hist_entry storage little more generic?
> and maybe dynamically allocated?

I'm fine with it.

> 
> I'm doing the same thing for the c2c stuff, like having
> for each hist_entry its own 'struct hists' object, which
> records data related to parent hist_entry
> 
> maybe we could strip the 'hists' object to some bare minimum
> which is needed for store/sort/stat and display entries
> in hists_browser ;-)

Yeah, as you can see, it basically needs a pointer to parent entry,
hpp output format, and rbtrees for chlidren.  Also I added depth and
leaf field to make it easy for browser routines.

> 
> I'm preparing RFC patchset in my perf/c2c branch, if you want
> to take a look

Sure.

> 
> however, as I said above, for my own sake it all boils down
> to have 'hists' object within hist_entry, so I can use it
> in the UI code easily
> 
> FYI I also added support for hists object's own sort/output
> format lists.. which I'll curve out and send for review soon

OK, will take a look.

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1312388 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

Em Tue, Jan 19, 2016 at 07:51:18PM +0900, Namhyung Kim escreveu:
> Hi Jiri,
> 
> On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> > On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> > 
> > SNIP
> > 
> > >  	char			*srcfile;
> > >  	struct symbol		*parent;
> > > -	struct rb_root		sorted_chain;
> > >  	struct branch_info	*branch_info;
> > >  	struct hists		*hists;
> > >  	struct mem_info		*mem_info;
> > >  	void			*raw_data;
> > >  	u32			raw_size;
> > >  	void			*trace_output;
> > > +	struct perf_hpp_fmt	*fmt;
> > > +	struct hist_entry	*parent_he;
> > > +	union {
> > > +		/* this is for hierarchical entry structure */
> > > +		struct {
> > > +			struct rb_root	hroot_in;
> > > +			struct rb_root  hroot_out;
> > > +		};				/* non-leaf entries */
> > > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > > +	};
> > 
> > looks like cool feature!
> 
> Thanks!
> 
> > 
> > could we have the hist_entry storage little more generic?
> > and maybe dynamically allocated?
> 
> I'm fine with it.

Ok, so how should we proceed? I propose I test this patchkit, which
indeed looks cool from this cover letter description, yay!

If I find no problems, I'll merge it and, then, on top of it, you guys
can work on having this per-feature priv storage sorted out?

Please advise, meanwhile I'll cherry-pick whatever seems easy from both
patchkits.

- Arnaldo
 
> > 
> > I'm doing the same thing for the c2c stuff, like having
> > for each hist_entry its own 'struct hists' object, which
> > records data related to parent hist_entry
> > 
> > maybe we could strip the 'hists' object to some bare minimum
> > which is needed for store/sort/stat and display entries
> > in hists_browser ;-)
> 
> Yeah, as you can see, it basically needs a pointer to parent entry,
> hpp output format, and rbtrees for chlidren.  Also I added depth and
> leaf field to make it easy for browser routines.
> 
> > 
> > I'm preparing RFC patchset in my perf/c2c branch, if you want
> > to take a look
> 
> Sure.
> 
> > 
> > however, as I said above, for my own sake it all boils down
> > to have 'hists' object within hist_entry, so I can use it
> > in the UI code easily
> > 
> > FYI I also added support for hists object's own sort/output
> > format lists.. which I'll curve out and send for review soon
> 
> OK, will take a look.
> 
> Thanks,
> Namhyung

[toc] | [prev] | [next] | [standalone]

#1313375 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

On Tue, Jan 19, 2016 at 01:50:47PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 19, 2016 at 07:51:18PM +0900, Namhyung Kim escreveu:
> > Hi Jiri,
> > 
> > On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> > > On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> > > 
> > > SNIP
> > > 
> > > >  	char			*srcfile;
> > > >  	struct symbol		*parent;
> > > > -	struct rb_root		sorted_chain;
> > > >  	struct branch_info	*branch_info;
> > > >  	struct hists		*hists;
> > > >  	struct mem_info		*mem_info;
> > > >  	void			*raw_data;
> > > >  	u32			raw_size;
> > > >  	void			*trace_output;
> > > > +	struct perf_hpp_fmt	*fmt;
> > > > +	struct hist_entry	*parent_he;
> > > > +	union {
> > > > +		/* this is for hierarchical entry structure */
> > > > +		struct {
> > > > +			struct rb_root	hroot_in;
> > > > +			struct rb_root  hroot_out;
> > > > +		};				/* non-leaf entries */
> > > > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > > > +	};
> > > 
> > > looks like cool feature!
> > 
> > Thanks!
> > 
> > > 
> > > could we have the hist_entry storage little more generic?
> > > and maybe dynamically allocated?
> > 
> > I'm fine with it.
> 
> Ok, so how should we proceed? I propose I test this patchkit, which
> indeed looks cool from this cover letter description, yay!
> 
> If I find no problems, I'll merge it and, then, on top of it, you guys
> can work on having this per-feature priv storage sorted out?
> 
> Please advise, meanwhile I'll cherry-pick whatever seems easy from both
> patchkits.

Namhyung,
are you going to send another version, or should I review this one?

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1313391 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

Em Wed, Jan 20, 2016 at 06:00:59PM +0100, Jiri Olsa escreveu:
> On Tue, Jan 19, 2016 at 01:50:47PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jan 19, 2016 at 07:51:18PM +0900, Namhyung Kim escreveu:
> > > Hi Jiri,
> > > 
> > > On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> > > > On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> > > > 
> > > > SNIP
> > > > 
> > > > >  	char			*srcfile;
> > > > >  	struct symbol		*parent;
> > > > > -	struct rb_root		sorted_chain;
> > > > >  	struct branch_info	*branch_info;
> > > > >  	struct hists		*hists;
> > > > >  	struct mem_info		*mem_info;
> > > > >  	void			*raw_data;
> > > > >  	u32			raw_size;
> > > > >  	void			*trace_output;
> > > > > +	struct perf_hpp_fmt	*fmt;
> > > > > +	struct hist_entry	*parent_he;
> > > > > +	union {
> > > > > +		/* this is for hierarchical entry structure */
> > > > > +		struct {
> > > > > +			struct rb_root	hroot_in;
> > > > > +			struct rb_root  hroot_out;
> > > > > +		};				/* non-leaf entries */
> > > > > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > > > > +	};
> > > > 
> > > > looks like cool feature!
> > > 
> > > Thanks!
> > > 
> > > > 
> > > > could we have the hist_entry storage little more generic?
> > > > and maybe dynamically allocated?
> > > 
> > > I'm fine with it.
> > 
> > Ok, so how should we proceed? I propose I test this patchkit, which
> > indeed looks cool from this cover letter description, yay!
> > 
> > If I find no problems, I'll merge it and, then, on top of it, you guys
> > can work on having this per-feature priv storage sorted out?
> > 
> > Please advise, meanwhile I'll cherry-pick whatever seems easy from both
> > patchkits.
> 
> Namhyung,
> are you going to send another version, or should I review this one?

This is what I am assuming, going thru the patches and replacing the
fourth (4/17) by the 4.1/17 and 4.2/17 that he sent.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1313851 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

Hi Jiri,

On Wed, Jan 20, 2016 at 06:00:59PM +0100, Jiri Olsa wrote:
> On Tue, Jan 19, 2016 at 01:50:47PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jan 19, 2016 at 07:51:18PM +0900, Namhyung Kim escreveu:
> > > Hi Jiri,
> > > 
> > > On Sun, Jan 17, 2016 at 05:15:33PM +0100, Jiri Olsa wrote:
> > > > On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> > > > 
> > > > SNIP
> > > > 
> > > > >  	char			*srcfile;
> > > > >  	struct symbol		*parent;
> > > > > -	struct rb_root		sorted_chain;
> > > > >  	struct branch_info	*branch_info;
> > > > >  	struct hists		*hists;
> > > > >  	struct mem_info		*mem_info;
> > > > >  	void			*raw_data;
> > > > >  	u32			raw_size;
> > > > >  	void			*trace_output;
> > > > > +	struct perf_hpp_fmt	*fmt;
> > > > > +	struct hist_entry	*parent_he;
> > > > > +	union {
> > > > > +		/* this is for hierarchical entry structure */
> > > > > +		struct {
> > > > > +			struct rb_root	hroot_in;
> > > > > +			struct rb_root  hroot_out;
> > > > > +		};				/* non-leaf entries */
> > > > > +		struct rb_root	sorted_chain;	/* leaf entry has callchains */
> > > > > +	};
> > > > 
> > > > looks like cool feature!
> > > 
> > > Thanks!
> > > 
> > > > 
> > > > could we have the hist_entry storage little more generic?
> > > > and maybe dynamically allocated?
> > > 
> > > I'm fine with it.
> > 
> > Ok, so how should we proceed? I propose I test this patchkit, which
> > indeed looks cool from this cover letter description, yay!
> > 
> > If I find no problems, I'll merge it and, then, on top of it, you guys
> > can work on having this per-feature priv storage sorted out?
> > 
> > Please advise, meanwhile I'll cherry-pick whatever seems easy from both
> > patchkits.
> 
> Namhyung,
> are you going to send another version, or should I review this one?

Please review this version!

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1314088 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:

SNIP

> +}
> +
> +static bool hists__hierarchy_insert_entry(struct hists *hists,
> +					  struct rb_root *root,
> +					  struct hist_entry *he)
> +{
> +	struct perf_hpp_fmt *fmt;
> +	struct hist_entry *new = NULL;
> +	struct hist_entry *parent = NULL;
> +	int depth = 0;
> +
> +	perf_hpp__for_each_sort_list(fmt) {
> +		if (!perf_hpp__is_sort_entry(fmt) &&
> +		    !perf_hpp__is_dynamic_entry(fmt))
> +			continue;
> +
> +		/* insert copy of 'he' for each fmt into the hierarchy */
> +		new = hierarchy_insert_entry(hists, root, he, fmt);
> +		if (new == NULL)
> +			break;

so hierarchy_insert_entry can fail because of memory allocation
but the resort path does not cover any error path because it only
shuffles entries from in-tree into sorted tree

would it make more sense to do this in 'in-tree addition' path?
and keep the resort functions to do only resort stuff

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1314169 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

Hi Jiri,

On Thu, Jan 21, 2016 at 11:43:30AM +0100, Jiri Olsa wrote:
> On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > +}
> > +
> > +static bool hists__hierarchy_insert_entry(struct hists *hists,
> > +					  struct rb_root *root,
> > +					  struct hist_entry *he)
> > +{
> > +	struct perf_hpp_fmt *fmt;
> > +	struct hist_entry *new = NULL;
> > +	struct hist_entry *parent = NULL;
> > +	int depth = 0;
> > +
> > +	perf_hpp__for_each_sort_list(fmt) {
> > +		if (!perf_hpp__is_sort_entry(fmt) &&
> > +		    !perf_hpp__is_dynamic_entry(fmt))
> > +			continue;
> > +
> > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > +		if (new == NULL)
> > +			break;
> 
> so hierarchy_insert_entry can fail because of memory allocation
> but the resort path does not cover any error path because it only
> shuffles entries from in-tree into sorted tree

Yes, memory allocation can fail anywhere.  If it happens, there's not
much thing we can do IMHO - just print warning and bail out.
Currently it silently ignores the allocation error and try to proceed.
But I guess it'll fail soon at other place anyway.

AFAICS current code also can fail in callchain_merge()..

Maybe we can change the return type of this function to int and treat
-1 as an error to detect such cases.


> 
> would it make more sense to do this in 'in-tree addition' path?
> and keep the resort functions to do only resort stuff

I don't follow.  There're 3 path to handle hist entries - let's say
them as 'addition', 'collapsing', and 'resort'.  This function does
the 'collapsing' part - it was originally intended to merge sharable
entries (namely for same 'comm' among different threads).  But I used
it to build a hierarchy since I found it useful as follows:

  1. it requires smaller change than doing it in the 'addition' path
  2. it can reuse current callback-based 'addition' paths so mem- and
     branch-mode can be supported easily (but it needs test..).
  3. the 'addition' path can be parallelized so it'll increase memory
     footprint if it build temporary local hierarchies during the path.

The 'resort' path always do sorting only..

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1314199 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:

SNIP

> > > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > > +		if (new == NULL)
> > > +			break;
> > 
> > so hierarchy_insert_entry can fail because of memory allocation
> > but the resort path does not cover any error path because it only
> > shuffles entries from in-tree into sorted tree
> 
> Yes, memory allocation can fail anywhere.  If it happens, there's not
> much thing we can do IMHO - just print warning and bail out.
> Currently it silently ignores the allocation error and try to proceed.
> But I guess it'll fail soon at other place anyway.

I thought the 'policy' is to handle all allocation failures

> 
> AFAICS current code also can fail in callchain_merge()..
> 
> Maybe we can change the return type of this function to int and treat
> -1 as an error to detect such cases.
> 
> 
> > 
> > would it make more sense to do this in 'in-tree addition' path?
> > and keep the resort functions to do only resort stuff
> 
> I don't follow.  There're 3 path to handle hist entries - let's say
> them as 'addition', 'collapsing', and 'resort'.  This function does
> the 'collapsing' part - it was originally intended to merge sharable
> entries (namely for same 'comm' among different threads).  But I used
> it to build a hierarchy since I found it useful as follows:
> 
>   1. it requires smaller change than doing it in the 'addition' path
>   2. it can reuse current callback-based 'addition' paths so mem- and
>      branch-mode can be supported easily (but it needs test..).
>   3. the 'addition' path can be parallelized so it'll increase memory
>      footprint if it build temporary local hierarchies during the path.
> 
> The 'resort' path always do sorting only..

well, you are adding/duplicating entries now in resort path
and that is not just 'sorting only'

you allow only sort and tracepoint entries to be added in
hierrarych view, so there's no resort needed, but still it
could be added in future? not sure

it still makes more sense to me to do this in 'addition' path,
because you basically add new entries

but have no other grounds for this also I might be missing something ;-)

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1314215 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

Em Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa escreveu:
> On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > > > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > > > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > > > +		if (new == NULL)
> > > > +			break;

Also, can we rename 'new' to new_he? In the past I used 'self' and
Thomas rightly told me that 'self' didn't convey any info, likewise for
'new' (that is even a keyword in C++ and may confuse some syntax
highligting, etc).

> > > so hierarchy_insert_entry can fail because of memory allocation
> > > but the resort path does not cover any error path because it only
> > > shuffles entries from in-tree into sorted tree

> > Yes, memory allocation can fail anywhere.  If it happens, there's not
> > much thing we can do IMHO - just print warning and bail out.
> > Currently it silently ignores the allocation error and try to proceed.
> > But I guess it'll fail soon at other place anyway.
> 
> I thought the 'policy' is to handle all allocation failures

yup, if some place doesn't, we need to fix it, silently trowing away
stuff is not good. At the very least count the number of failures and
inform the user somewhere on the screen.
 
> > AFAICS current code also can fail in callchain_merge()..

That needs to be fixed too, then.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1314886 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

Hi Arnaldo,

On Thu, Jan 21, 2016 at 11:02:09AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa escreveu:
> > On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> > 
> > SNIP
> > 
> > > > > +		/* insert copy of 'he' for each fmt into the hierarchy */
> > > > > +		new = hierarchy_insert_entry(hists, root, he, fmt);
> > > > > +		if (new == NULL)
> > > > > +			break;
> 
> Also, can we rename 'new' to new_he? In the past I used 'self' and
> Thomas rightly told me that 'self' didn't convey any info, likewise for
> 'new' (that is even a keyword in C++ and may confuse some syntax
> highligting, etc).

OK


> 
> > > > so hierarchy_insert_entry can fail because of memory allocation
> > > > but the resort path does not cover any error path because it only
> > > > shuffles entries from in-tree into sorted tree
> 
> > > Yes, memory allocation can fail anywhere.  If it happens, there's not
> > > much thing we can do IMHO - just print warning and bail out.
> > > Currently it silently ignores the allocation error and try to proceed.
> > > But I guess it'll fail soon at other place anyway.
> > 
> > I thought the 'policy' is to handle all allocation failures
> 
> yup, if some place doesn't, we need to fix it, silently trowing away
> stuff is not good. At the very least count the number of failures and
> inform the user somewhere on the screen.

OK, will change.


>  
> > > AFAICS current code also can fail in callchain_merge()..
> 
> That needs to be fixed too, then.

OK

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1314883 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

Hi Jiri,

On Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa wrote:
> On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> > > would it make more sense to do this in 'in-tree addition' path?
> > > and keep the resort functions to do only resort stuff
> > 
> > I don't follow.  There're 3 path to handle hist entries - let's say
> > them as 'addition', 'collapsing', and 'resort'.  This function does
> > the 'collapsing' part - it was originally intended to merge sharable
> > entries (namely for same 'comm' among different threads).  But I used
> > it to build a hierarchy since I found it useful as follows:
> > 
> >   1. it requires smaller change than doing it in the 'addition' path
> >   2. it can reuse current callback-based 'addition' paths so mem- and
> >      branch-mode can be supported easily (but it needs test..).
> >   3. the 'addition' path can be parallelized so it'll increase memory
> >      footprint if it build temporary local hierarchies during the path.
> > 
> > The 'resort' path always do sorting only..
> 
> well, you are adding/duplicating entries now in resort path
> and that is not just 'sorting only'

As I said, this is not the 'resort' path, the 'resort' path is the
subject of patch 02/17.


> 
> you allow only sort and tracepoint entries to be added in
> hierrarych view, so there's no resort needed, but still it
> could be added in future? not sure

Resorting is still needed since it should sort entries by overhead
(period).  Anyway, in this 'collapsing' path, we need to build a
hierarchy to be sorted at the resort path.


> 
> it still makes more sense to me to do this in 'addition' path,
> because you basically add new entries
> 
> but have no other grounds for this also I might be missing something ;-)

What about thinking like this?

1. addition   - add samples into hist entries
2. collapsing - build complete hists (hierarchy) to be sorted
3. resort     - sort final entries based on the sort keys

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1314922 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

On Fri, Jan 22, 2016 at 07:43:12PM +0900, Namhyung Kim wrote:
> Hi Jiri,
> 
> On Thu, Jan 21, 2016 at 02:35:58PM +0100, Jiri Olsa wrote:
> > On Thu, Jan 21, 2016 at 09:55:52PM +0900, Namhyung Kim wrote:
> > > > would it make more sense to do this in 'in-tree addition' path?
> > > > and keep the resort functions to do only resort stuff
> > > 
> > > I don't follow.  There're 3 path to handle hist entries - let's say
> > > them as 'addition', 'collapsing', and 'resort'.  This function does
> > > the 'collapsing' part - it was originally intended to merge sharable
> > > entries (namely for same 'comm' among different threads).  But I used
> > > it to build a hierarchy since I found it useful as follows:
> > > 
> > >   1. it requires smaller change than doing it in the 'addition' path
> > >   2. it can reuse current callback-based 'addition' paths so mem- and
> > >      branch-mode can be supported easily (but it needs test..).
> > >   3. the 'addition' path can be parallelized so it'll increase memory
> > >      footprint if it build temporary local hierarchies during the path.
> > > 
> > > The 'resort' path always do sorting only..
> > 
> > well, you are adding/duplicating entries now in resort path
> > and that is not just 'sorting only'
> 
> As I said, this is not the 'resort' path, the 'resort' path is the
> subject of patch 02/17.

ok, I took the collapsing as a part of resort

> 
> 
> > 
> > you allow only sort and tracepoint entries to be added in
> > hierrarych view, so there's no resort needed, but still it
> > could be added in future? not sure
> 
> Resorting is still needed since it should sort entries by overhead
> (period).  Anyway, in this 'collapsing' path, we need to build a
> hierarchy to be sorted at the resort path.
> 
> 
> > 
> > it still makes more sense to me to do this in 'addition' path,
> > because you basically add new entries
> > 
> > but have no other grounds for this also I might be missing something ;-)
> 
> What about thinking like this?
> 
> 1. addition   - add samples into hist entries
> 2. collapsing - build complete hists (hierarchy) to be sorted
> 3. resort     - sort final entries based on the sort keys

ok, sounds good.. maybe we could use some other name
for collapsing then.. like build,setup or such

thanks for bearing with me ;-)
jirka

[toc] | [prev] | [next] | [standalone]

#1314122 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:

SNIP

> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 687bbb124428..15a75d44de91 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -94,9 +94,11 @@ struct hist_entry {
>  	s32			socket;
>  	s32			cpu;
>  	u8			cpumode;
> +	u8			depth;
>  
>  	/* We are added by hists__add_dummy_entry. */
>  	bool			dummy;
> +	bool			leaf;
>  
>  	char			level;
>  	u8			filtered;
> @@ -118,13 +120,22 @@ struct hist_entry {
>  	char			*srcline;
>  	char			*srcfile;
>  	struct symbol		*parent;
> -	struct rb_root		sorted_chain;
>  	struct branch_info	*branch_info;
>  	struct hists		*hists;
>  	struct mem_info		*mem_info;
>  	void			*raw_data;
>  	u32			raw_size;
>  	void			*trace_output;
> +	struct perf_hpp_fmt	*fmt;
> +	struct hist_entry	*parent_he;
> +	union {
> +		/* this is for hierarchical entry structure */
> +		struct {
> +			struct rb_root	hroot_in;
> +			struct rb_root  hroot_out;

we use 'entries_in' and 'entries' in hists object

could we keep the names in here as well to indicate
it's doing the same stuff..?

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1314188 — Re: [PATCH 01/17] perf hists: Basic support of hierarchical report view

On Thu, Jan 21, 2016 at 12:35:11PM +0100, Jiri Olsa wrote:
> On Sun, Jan 17, 2016 at 01:03:01AM +0900, Namhyung Kim wrote:
> 
> SNIP
> 
> > diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> > index 687bbb124428..15a75d44de91 100644
> > --- a/tools/perf/util/sort.h
> > +++ b/tools/perf/util/sort.h
> > @@ -94,9 +94,11 @@ struct hist_entry {
> >  	s32			socket;
> >  	s32			cpu;
> >  	u8			cpumode;
> > +	u8			depth;
> >  
> >  	/* We are added by hists__add_dummy_entry. */
> >  	bool			dummy;
> > +	bool			leaf;
> >  
> >  	char			level;
> >  	u8			filtered;
> > @@ -118,13 +120,22 @@ struct hist_entry {
> >  	char			*srcline;
> >  	char			*srcfile;
> >  	struct symbol		*parent;
> > -	struct rb_root		sorted_chain;
> >  	struct branch_info	*branch_info;
> >  	struct hists		*hists;
> >  	struct mem_info		*mem_info;
> >  	void			*raw_data;
> >  	u32			raw_size;
> >  	void			*trace_output;
> > +	struct perf_hpp_fmt	*fmt;
> > +	struct hist_entry	*parent_he;
> > +	union {
> > +		/* this is for hierarchical entry structure */
> > +		struct {
> > +			struct rb_root	hroot_in;
> > +			struct rb_root  hroot_out;
> 
> we use 'entries_in' and 'entries' in hists object
> 
> could we keep the names in here as well to indicate
> it's doing the same stuff..?

Maybe it's a preference.  I don't think it's necessary, sometimes we
need to differentiate whether it accesses to (top-level) hists or hist
entry IMHO.

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

Page 1 of 4  [1] 2 3 4  Next page →

Back to top | Article view | linux.kernel

csiph-web