LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Liang, Kan" <kan.liang@intel.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: "jolsa@redhat.com" <jolsa@redhat.com>,
	"namhyung@kernel.org" <namhyung@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"ak@linux.intel.com" <ak@linux.intel.com>
Subject: RE: [PATCH V6 1/1] perf tool: perf diff support for different binaries
Date: Tue, 6 Jan 2015 16:39:20 +0000	[thread overview]
Message-ID: <37D7C6CF3E00A74B8858931C1DB2F07701693397@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <20150106145356.GA10675@kernel.org>



> Em Tue, Dec 02, 2014 at 10:39:18AM -0500, kan.liang@intel.com escreveu:
> > From: Kan Liang <kan.liang@intel.com>
> >
> > Currently, the perf diff only works with same binaries. That's because
> > it compares the symbol start address. It doesn't work if the perf.data
> > comes from different binaries. This patch matches the function names.
> 
> Sorry Kan, I saw these patches, its just that it the above statement looked
> completely strange to me, i.e. of course it should look at the dso name,
> then at the symbol name, comparing via addresses makes no sense to me,
> so I kept leaving this patch to look after processing other patches and doing
> other things, then the year end holidays, etc.
> 
> So now I'm looking at old code, from 2009:
> 
>   commit 604c5c92972dcb4b98be34775452d09a5d4ec248
>   Author: Arnaldo Carvalho de Melo <acme@redhat.com>
>   Date:   Wed Dec 16 14:09:53 2009 -0200
> 
>     perf diff: Change the default sort order to "dso,symbol"
> 
>     This is a more intuitive / more meaningful default:
> 
>     $ perf diff | head -8
>          9.02%     +1.00%  libc-2.10.1.so               [.] _IO_vfprintf_internal
>          2.91%     -1.00%  [kernel]                     [k] __kmalloc
>          2.85%     -1.00%  [kernel]                     [k] ext4_htree_store_dirent
>          1.99%     -1.00%  [kernel]                     [k] _atomic_dec_and_lock
>          2.44%             [kernel]
>     $
> 
>     Suggested-by: Ingo Molnar <mingo@elte.hu>
> 
> and I see:
> 
> static void perf_session__match_hists(struct perf_session *old_session,
>                                       struct perf_session *new_session) {
>         struct rb_node *nd;
> 
>         perf_session__resort_by_name(old_session);
> 
>         for (nd = rb_first(&new_session->hists); nd; nd = rb_next(nd)) {
>                 struct hist_entry *pos = rb_entry(nd, struct hist_entry, rb_node);
>                 pos->pair = perf_session__find_hist_entry_by_name(old_session,
> pos);
>         }
> }
> 
> Ok, it will process all samples, store everything in hist_entries, etc, then
> traverse all entries in the most recent perf.data file and look for a pair, _by
> name_:
> 
> static struct hist_entry *
> perf_session__find_hist_entry_by_name(struct perf_session *self,
>                                       struct hist_entry *he) {
>         struct rb_node *n = self->hists.rb_node;
> 
>         while (n) {
>                 struct hist_entry *iter = rb_entry(n, struct hist_entry, rb_node);
>                 int cmp = strcmp(he->map->dso->name, iter->map->dso->name);
> 
>                 if (cmp > 0)
>                         n = n->rb_left;
>                 else if (cmp < 0)
>                         n = n->rb_right;
>                 else {
>                         cmp = strcmp(he->sym->name, iter->sym->name);
>                         if (cmp > 0)
>                                 n = n->rb_left;
>                         else if (cmp < 0)
>                                 n = n->rb_right;
>                         else
>                                 return iter;
>                 }
>         }
> 
>         return NULL;
> }
> 
> See? Resort the old session by "dso,symbol_name", then go on pairing
> them.
> 
> So at some point this got broken :-\
> 
> I.e. this is not changing an existing correct behaviour, but fixing a bug, i.e.
> the changeset comment confused me :-\
> 
> Now I'm trying to figure out _when_ this got broken, what was the
> reasoning for that code to have changed in a way that made it not match
> by dso_name, symbol_name.
> 


It looks it got broken just after two weeks later in 2009. 
perf_session__find_hist_entry_by_name was replaced by hist_entry__cmp.
After that it doesn't compares the name any more.


commit 9c443dfdd31eddea6cbe6ee0ca469fbcc4e1dc3b
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Mon Dec 28 22:48:36 2009 -0200

    perf diff: Fix support for all --sort combinations

@@ -122,29 +112,28 @@ static void perf_session__resort_by_name(struct perf_session *self)
        self->hists = tmp;
 }

+static void perf_session__set_hist_entries_positions(struct perf_session *self)
+{
+       perf_session__output_resort(self, self->events_stats.total);
+       perf_session__resort_hist_entries(self);
+}
+
 static struct hist_entry *
-perf_session__find_hist_entry_by_name(struct perf_session *self,
-                                     struct hist_entry *he)
+perf_session__find_hist_entry(struct perf_session *self,
+                             struct hist_entry *he)
 {
        struct rb_node *n = self->hists.rb_node;

        while (n) {
                struct hist_entry *iter = rb_entry(n, struct hist_entry, rb_node);
-               int cmp = strcmp(he->map->dso->name, iter->map->dso->name);
+               int64_t cmp = hist_entry__cmp(he, iter);

-               if (cmp > 0)
+               if (cmp < 0)
                        n = n->rb_left;
-               else if (cmp < 0)
+               else if (cmp > 0)
                        n = n->rb_right;
-               else {
-                       cmp = strcmp(he->sym->name, iter->sym->name);
-                       if (cmp > 0)
-                               n = n->rb_left;
-                       else if (cmp < 0)
-                               n = n->rb_right;
-                       else
-                               return iter;
-               }
+               else
+                       return iter;
        }

        return NULL;
@@ -155,11 +144,9 @@ static void perf_session__match_hists(struct perf_session *old_session,
 {
        struct rb_node *nd;

-       perf_session__resort_by_name(old_session);
-
        for (nd = rb_first(&new_session->hists); nd; nd = rb_next(nd)) {
                struct hist_entry *pos = rb_entry(nd, struct hist_entry, rb_node);
-               pos->pair = perf_session__find_hist_entry_by_name(old_session, pos);
+               pos->pair = perf_session__find_hist_entry(old_session, pos);
        }
 }





  parent reply	other threads:[~2015-01-06 16:39 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-02 15:39 kan.liang
2015-01-05 16:04 ` Liang, Kan
2015-01-06 14:53 ` Arnaldo Carvalho de Melo
2015-01-06 16:16   ` Arnaldo Carvalho de Melo
2015-01-12 18:06     ` Liang, Kan
2015-01-06 16:39   ` Liang, Kan [this message]
2015-01-26 14:15   ` Liang, Kan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=37D7C6CF3E00A74B8858931C1DB2F07701693397@SHSMSX103.ccr.corp.intel.com \
    --to=kan.liang@intel.com \
    --cc=acme@kernel.org \
    --cc=ak@linux.intel.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=namhyung@kernel.org \
    --subject='RE: [PATCH V6 1/1] perf tool: perf diff support for different binaries' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).